diff --git a/backend/cmd/server/main.go b/backend/cmd/server/main.go index 46edcb69..43c35797 100644 --- a/backend/cmd/server/main.go +++ b/backend/cmd/server/main.go @@ -56,7 +56,7 @@ func init() { // In non-release mode, Debug level logs are enabled. func main() { logger.InitBootstrap() - defer logger.Sync() + defer logger.Shutdown() // Parse command line flags setupMode := flag.Bool("setup", false, "Run setup wizard in CLI mode") diff --git a/backend/internal/handler/admin/admin_basic_handlers_test.go b/backend/internal/handler/admin/admin_basic_handlers_test.go index 409aa966..73d10190 100644 --- a/backend/internal/handler/admin/admin_basic_handlers_test.go +++ b/backend/internal/handler/admin/admin_basic_handlers_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "testing" + "github.com/Wei-Shaw/sub2api/internal/service" "github.com/gin-gonic/gin" "github.com/stretchr/testify/require" ) @@ -226,7 +227,13 @@ func TestProxyHandlerEndpoints(t *testing.T) { } func TestRedeemHandlerEndpoints(t *testing.T) { - router, _ := setupAdminRouter() + router, adminSvc := setupAdminRouter() + adminSvc.batchDeleteRedeemResult = &service.RedeemBatchDeleteResult{ + DeletedIDs: []int64{1}, + Skipped: []service.RedeemBatchDeleteSkipped{ + {ID: 2, Reason: "db error"}, + }, + } rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodGet, "/api/v1/admin/redeem-codes", nil) @@ -255,6 +262,20 @@ func TestRedeemHandlerEndpoints(t *testing.T) { req.Header.Set("Content-Type", "application/json") router.ServeHTTP(rec, req) require.Equal(t, http.StatusOK, rec.Code) + var resp struct { + Code int `json:"code"` + Data struct { + DeletedIDs []int64 `json:"deleted_ids"` + Skipped []struct { + ID int64 `json:"id"` + Reason string `json:"reason"` + } `json:"skipped"` + } `json:"data"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) + require.Equal(t, []int64{1}, resp.Data.DeletedIDs) + require.Len(t, resp.Data.Skipped, 1) + require.Equal(t, int64(2), resp.Data.Skipped[0].ID) rec = httptest.NewRecorder() req = httptest.NewRequest(http.MethodPost, "/api/v1/admin/redeem-codes/5/expire", nil) diff --git a/backend/internal/handler/admin/admin_service_stub_test.go b/backend/internal/handler/admin/admin_service_stub_test.go index 6d1ef1b6..47e269e4 100644 --- a/backend/internal/handler/admin/admin_service_stub_test.go +++ b/backend/internal/handler/admin/admin_service_stub_test.go @@ -58,7 +58,8 @@ type stubAdminService struct { sortOrder string calls int } - mu sync.Mutex + batchDeleteRedeemResult *service.RedeemBatchDeleteResult + mu sync.Mutex } func newStubAdminService() *stubAdminService { @@ -449,8 +450,11 @@ func (s *stubAdminService) DeleteRedeemCode(ctx context.Context, id int64) error return nil } -func (s *stubAdminService) BatchDeleteRedeemCodes(ctx context.Context, ids []int64) (int64, error) { - return int64(len(ids)), nil +func (s *stubAdminService) BatchDeleteRedeemCodes(ctx context.Context, ids []int64) (*service.RedeemBatchDeleteResult, error) { + if s.batchDeleteRedeemResult != nil { + return s.batchDeleteRedeemResult, nil + } + return &service.RedeemBatchDeleteResult{DeletedIDs: ids, Skipped: []service.RedeemBatchDeleteSkipped{}}, nil } func (s *stubAdminService) ExpireRedeemCode(ctx context.Context, id int64) (*service.RedeemCode, error) { diff --git a/backend/internal/handler/admin/redeem_handler.go b/backend/internal/handler/admin/redeem_handler.go index 24365f3d..901a0904 100644 --- a/backend/internal/handler/admin/redeem_handler.go +++ b/backend/internal/handler/admin/redeem_handler.go @@ -249,16 +249,13 @@ func (h *RedeemHandler) BatchDelete(c *gin.Context) { return } - deleted, err := h.adminService.BatchDeleteRedeemCodes(c.Request.Context(), req.IDs) + result, err := h.adminService.BatchDeleteRedeemCodes(c.Request.Context(), req.IDs) if err != nil { response.ErrorFrom(c, err) return } - response.Success(c, gin.H{ - "deleted": deleted, - "message": "Redeem codes deleted successfully", - }) + response.Success(c, result) } // Expire handles expiring a redeem code diff --git a/backend/internal/pkg/logger/logger.go b/backend/internal/pkg/logger/logger.go index 3fca706e..5ec96284 100644 --- a/backend/internal/pkg/logger/logger.go +++ b/backend/internal/pkg/logger/logger.go @@ -48,6 +48,7 @@ var ( atomicLevel zap.AtomicLevel initOptions InitOptions currentSink atomic.Value // sinkState + currentClose func() stdLogUndo func() bootstrapOnce sync.Once ) @@ -72,16 +73,18 @@ func Init(options InitOptions) error { func initLocked(options InitOptions) error { normalized := options.normalized() - zl, al, err := buildLogger(normalized) + zl, al, closeFn, err := buildLogger(normalized) if err != nil { return err } prev := global.Load() + prevClose := currentClose global.Store(zl) sugar.Store(zl.Sugar()) atomicLevel = al initOptions = normalized + currentClose = closeFn bridgeSlogLocked() bridgeStdLogLocked() @@ -89,6 +92,9 @@ func initLocked(options InitOptions) error { if prev != nil { _ = prev.Sync() } + if prevClose != nil { + prevClose() + } return nil } @@ -205,6 +211,27 @@ func Sync() { } } +func Shutdown() { + mu.Lock() + defer mu.Unlock() + + if stdLogUndo != nil { + stdLogUndo() + stdLogUndo = nil + } + + if l := global.Load(); l != nil { + _ = l.Sync() + } + if currentClose != nil { + currentClose() + currentClose = nil + } + + global.Store(nil) + sugar.Store(nil) +} + func bridgeStdLogLocked() { if stdLogUndo != nil { stdLogUndo() @@ -238,7 +265,7 @@ func bridgeSlogLocked() { slog.SetDefault(slog.New(newSlogZapHandler(base.Named("slog")))) } -func buildLogger(options InitOptions) (*zap.Logger, zap.AtomicLevel, error) { +func buildLogger(options InitOptions) (*zap.Logger, zap.AtomicLevel, func(), error) { level, _ := parseLevel(options.Level) atomic := zap.NewAtomicLevelAt(level) @@ -265,6 +292,7 @@ func buildLogger(options InitOptions) (*zap.Logger, zap.AtomicLevel, error) { sinkCore := newSinkCore() cores := make([]zapcore.Core, 0, 3) + var closers []io.Closer if options.Output.ToStdout { infoPriority := zap.LevelEnablerFunc(func(lvl zapcore.Level) bool { @@ -273,12 +301,12 @@ func buildLogger(options InitOptions) (*zap.Logger, zap.AtomicLevel, error) { errPriority := zap.LevelEnablerFunc(func(lvl zapcore.Level) bool { return lvl >= atomic.Level() && lvl >= zapcore.WarnLevel }) - cores = append(cores, zapcore.NewCore(enc, zapcore.Lock(os.Stdout), infoPriority)) - cores = append(cores, zapcore.NewCore(enc, zapcore.Lock(os.Stderr), errPriority)) + cores = append(cores, zapcore.NewCore(enc, stdStreamWriteSyncer(os.Stdout), infoPriority)) + cores = append(cores, zapcore.NewCore(enc, stdStreamWriteSyncer(os.Stderr), errPriority)) } if options.Output.ToFile { - fileCore, filePath, fileErr := buildFileCore(enc, atomic, options) + fileCore, filePath, fileCloser, fileErr := buildFileCore(enc, atomic, options) if fileErr != nil { _, _ = fmt.Fprintf(os.Stderr, "time=%s level=WARN msg=\"日志文件输出初始化失败，降级为仅标准输出\" path=%s err=%v\n", time.Now().Format(time.RFC3339Nano), @@ -287,11 +315,12 @@ func buildLogger(options InitOptions) (*zap.Logger, zap.AtomicLevel, error) { ) } else { cores = append(cores, fileCore) + closers = append(closers, fileCloser) } } if len(cores) == 0 { - cores = append(cores, zapcore.NewCore(enc, zapcore.Lock(os.Stdout), atomic)) + cores = append(cores, zapcore.NewCore(enc, stdStreamWriteSyncer(os.Stdout), atomic)) } core := zapcore.NewTee(cores...) @@ -313,10 +342,14 @@ func buildLogger(options InitOptions) (*zap.Logger, zap.AtomicLevel, error) { zap.String("service", options.ServiceName), zap.String("env", options.Environment), ) - return logger, atomic, nil + return logger, atomic, func() { + for _, closer := range closers { + _ = closer.Close() + } + }, nil } -func buildFileCore(enc zapcore.Encoder, atomic zap.AtomicLevel, options InitOptions) (zapcore.Core, string, error) { +func buildFileCore(enc zapcore.Encoder, atomic zap.AtomicLevel, options InitOptions) (zapcore.Core, string, io.Closer, error) { filePath := options.Output.FilePath if strings.TrimSpace(filePath) == "" { filePath = resolveLogFilePath("") @@ -324,7 +357,7 @@ func buildFileCore(enc zapcore.Encoder, atomic zap.AtomicLevel, options InitOpti dir := filepath.Dir(filePath) if err := os.MkdirAll(dir, 0o755); err != nil { - return nil, filePath, err + return nil, filePath, nil, err } lj := &lumberjack.Logger{ Filename: filePath, @@ -334,7 +367,25 @@ func buildFileCore(enc zapcore.Encoder, atomic zap.AtomicLevel, options InitOpti Compress: options.Rotation.Compress, LocalTime: options.Rotation.LocalTime, } - return zapcore.NewCore(enc, zapcore.AddSync(lj), atomic), filePath, nil + return zapcore.NewCore(enc, zapcore.AddSync(lj), atomic), filePath, lj, nil +} + +type stdStreamSyncer struct { + file *os.File +} + +func stdStreamWriteSyncer(file *os.File) zapcore.WriteSyncer { + return zapcore.Lock(&stdStreamSyncer{file: file}) +} + +func (s *stdStreamSyncer) Write(p []byte) (int, error) { + return s.file.Write(p) +} + +func (s *stdStreamSyncer) Sync() error { + // Standard streams do not need fsync semantics, and on Windows a pipe-backed + // stdout/stderr can block indefinitely in FlushFileBuffers. + return nil } type sinkCore struct { diff --git a/backend/internal/pkg/logger/logger_test.go b/backend/internal/pkg/logger/logger_test.go index 74aae061..8e847c74 100644 --- a/backend/internal/pkg/logger/logger_test.go +++ b/backend/internal/pkg/logger/logger_test.go @@ -33,6 +33,7 @@ func TestInit_DualOutput(t *testing.T) { _ = stdoutW.Close() _ = stderrW.Close() }) + t.Cleanup(Shutdown) err = Init(InitOptions{ Level: "debug", @@ -103,6 +104,7 @@ func TestInit_FileOutputFailureDowngrade(t *testing.T) { _ = stderrR.Close() _ = stderrW.Close() }) + t.Cleanup(Shutdown) err = Init(InitOptions{ Level: "info", @@ -149,6 +151,7 @@ func TestInit_CallerShouldPointToCallsite(t *testing.T) { _ = stdoutW.Close() _ = stderrW.Close() }) + t.Cleanup(Shutdown) if err := Init(InitOptions{ Level: "info", diff --git a/backend/internal/pkg/logger/options_test.go b/backend/internal/pkg/logger/options_test.go index 10d50d72..876316f8 100644 --- a/backend/internal/pkg/logger/options_test.go +++ b/backend/internal/pkg/logger/options_test.go @@ -95,7 +95,7 @@ func TestBuildFileCore_InvalidPathFallback(t *testing.T) { EncodeLevel: zapcore.CapitalLevelEncoder, } encoder := zapcore.NewJSONEncoder(encoderCfg) - _, _, err := buildFileCore(encoder, zap.NewAtomicLevel(), opts) + _, _, _, err := buildFileCore(encoder, zap.NewAtomicLevel(), opts) if err == nil { t.Fatalf("buildFileCore() expected error for invalid path") } diff --git a/backend/internal/pkg/logger/stdlog_bridge_test.go b/backend/internal/pkg/logger/stdlog_bridge_test.go index 4482a2ec..02fd4c50 100644 --- a/backend/internal/pkg/logger/stdlog_bridge_test.go +++ b/backend/internal/pkg/logger/stdlog_bridge_test.go @@ -59,6 +59,7 @@ func TestStdLogBridgeRoutesLevels(t *testing.T) { _ = stderrR.Close() _ = stderrW.Close() }) + t.Cleanup(Shutdown) if err := Init(InitOptions{ Level: "debug", @@ -121,6 +122,7 @@ func TestLegacyPrintfRoutesLevels(t *testing.T) { _ = stderrR.Close() _ = stderrW.Close() }) + t.Cleanup(Shutdown) if err := Init(InitOptions{ Level: "debug", diff --git a/backend/internal/service/admin_service.go b/backend/internal/service/admin_service.go index 97b42c24..b9810e95 100644 --- a/backend/internal/service/admin_service.go +++ b/backend/internal/service/admin_service.go @@ -97,7 +97,7 @@ type AdminService interface { GetRedeemCode(ctx context.Context, id int64) (*RedeemCode, error) GenerateRedeemCodes(ctx context.Context, input *GenerateRedeemCodesInput) ([]RedeemCode, error) DeleteRedeemCode(ctx context.Context, id int64) error - BatchDeleteRedeemCodes(ctx context.Context, ids []int64) (int64, error) + BatchDeleteRedeemCodes(ctx context.Context, ids []int64) (*RedeemBatchDeleteResult, error) ExpireRedeemCode(ctx context.Context, id int64) (*RedeemCode, error) ResetAccountQuota(ctx context.Context, id int64) error } @@ -321,6 +321,16 @@ type ProxyBatchDeleteSkipped struct { Reason string `json:"reason"` } +type RedeemBatchDeleteResult struct { + DeletedIDs []int64 `json:"deleted_ids"` + Skipped []RedeemBatchDeleteSkipped `json:"skipped"` +} + +type RedeemBatchDeleteSkipped struct { + ID int64 `json:"id"` + Reason string `json:"reason"` +} + // ProxyTestResult represents the result of testing a proxy type ProxyTestResult struct { Success bool `json:"success"` @@ -2089,11 +2099,12 @@ func (s *adminServiceImpl) GenerateRedeemCodes(ctx context.Context, input *Gener code.ValidityDays = 30 // 默认30天 } } - if err := s.redeemCodeRepo.Create(ctx, &code); err != nil { - return nil, err - } codes = append(codes, code) } + + if err := s.redeemCodeRepo.CreateBatch(ctx, codes); err != nil { + return nil, err + } return codes, nil } @@ -2101,14 +2112,22 @@ func (s *adminServiceImpl) DeleteRedeemCode(ctx context.Context, id int64) error return s.redeemCodeRepo.Delete(ctx, id) } -func (s *adminServiceImpl) BatchDeleteRedeemCodes(ctx context.Context, ids []int64) (int64, error) { - var deleted int64 +func (s *adminServiceImpl) BatchDeleteRedeemCodes(ctx context.Context, ids []int64) (*RedeemBatchDeleteResult, error) { + result := &RedeemBatchDeleteResult{ + DeletedIDs: make([]int64, 0, len(ids)), + Skipped: make([]RedeemBatchDeleteSkipped, 0), + } for _, id := range ids { if err := s.redeemCodeRepo.Delete(ctx, id); err == nil { - deleted++ + result.DeletedIDs = append(result.DeletedIDs, id) + } else { + result.Skipped = append(result.Skipped, RedeemBatchDeleteSkipped{ + ID: id, + Reason: err.Error(), + }) } } - return deleted, nil + return result, nil } func (s *adminServiceImpl) ExpireRedeemCode(ctx context.Context, id int64) (*RedeemCode, error) { diff --git a/backend/internal/service/admin_service_delete_test.go b/backend/internal/service/admin_service_delete_test.go index fbc856cf..8c9d3dcd 100644 --- a/backend/internal/service/admin_service_delete_test.go +++ b/backend/internal/service/admin_service_delete_test.go @@ -248,8 +248,10 @@ func (s *proxyRepoStub) ListAccountSummariesByProxyID(ctx context.Context, proxy } type redeemRepoStub struct { - deleteErrByID map[int64]error - deletedIDs []int64 + createBatchErr error + createdBatches [][]RedeemCode + deleteErrByID map[int64]error + deletedIDs []int64 } func (s *redeemRepoStub) Create(ctx context.Context, code *RedeemCode) error { @@ -257,7 +259,18 @@ func (s *redeemRepoStub) Create(ctx context.Context, code *RedeemCode) error { } func (s *redeemRepoStub) CreateBatch(ctx context.Context, codes []RedeemCode) error { - panic("unexpected CreateBatch call") + if s.createBatchErr != nil { + return s.createBatchErr + } + cloned := append([]RedeemCode(nil), codes...) + s.createdBatches = append(s.createdBatches, cloned) + for i := range codes { + codes[i].ID = int64(i + 1) + if codes[i].CreatedAt.IsZero() { + codes[i].CreatedAt = time.Unix(int64(i+1), 0).UTC() + } + } + return nil } func (s *redeemRepoStub) GetByID(ctx context.Context, id int64) (*RedeemCode, error) { @@ -521,13 +534,45 @@ func TestAdminService_DeleteRedeemCode_Error(t *testing.T) { require.Equal(t, []int64{1}, repo.deletedIDs) } +func TestAdminService_GenerateRedeemCodes_UsesBatchCreate(t *testing.T) { + repo := &redeemRepoStub{} + svc := &adminServiceImpl{redeemCodeRepo: repo} + + codes, err := svc.GenerateRedeemCodes(context.Background(), &GenerateRedeemCodesInput{ + Count: 2, + Type: RedeemTypeBalance, + Value: 10, + }) + require.NoError(t, err) + require.Len(t, repo.createdBatches, 1) + require.Len(t, repo.createdBatches[0], 2) + require.Len(t, codes, 2) + require.NotZero(t, codes[0].ID) + require.Equal(t, StatusUnused, codes[0].Status) +} + +func TestAdminService_GenerateRedeemCodes_BatchCreateError(t *testing.T) { + repo := &redeemRepoStub{createBatchErr: errors.New("batch create failed")} + svc := &adminServiceImpl{redeemCodeRepo: repo} + + codes, err := svc.GenerateRedeemCodes(context.Background(), &GenerateRedeemCodesInput{ + Count: 2, + Type: RedeemTypeBalance, + Value: 10, + }) + require.Nil(t, codes) + require.ErrorContains(t, err, "batch create failed") + require.Len(t, repo.createdBatches, 0) +} + func TestAdminService_BatchDeleteRedeemCodes_Success(t *testing.T) { repo := &redeemRepoStub{} svc := &adminServiceImpl{redeemCodeRepo: repo} - deleted, err := svc.BatchDeleteRedeemCodes(context.Background(), []int64{1, 2, 3}) + result, err := svc.BatchDeleteRedeemCodes(context.Background(), []int64{1, 2, 3}) require.NoError(t, err) - require.Equal(t, int64(3), deleted) + require.Equal(t, []int64{1, 2, 3}, result.DeletedIDs) + require.Empty(t, result.Skipped) require.Equal(t, []int64{1, 2, 3}, repo.deletedIDs) } @@ -539,8 +584,11 @@ func TestAdminService_BatchDeleteRedeemCodes_PartialFailures(t *testing.T) { } svc := &adminServiceImpl{redeemCodeRepo: repo} - deleted, err := svc.BatchDeleteRedeemCodes(context.Background(), []int64{1, 2, 3}) + result, err := svc.BatchDeleteRedeemCodes(context.Background(), []int64{1, 2, 3}) require.NoError(t, err) - require.Equal(t, int64(2), deleted) + require.Equal(t, []int64{1, 3}, result.DeletedIDs) + require.Len(t, result.Skipped, 1) + require.Equal(t, int64(2), result.Skipped[0].ID) + require.Equal(t, "db error", result.Skipped[0].Reason) require.Equal(t, []int64{1, 2, 3}, repo.deletedIDs) } diff --git a/docs/reviews/2026-04-20-code-review-report.md b/docs/reviews/2026-04-20-code-review-report.md new file mode 100644 index 00000000..fae4e7bb --- /dev/null +++ b/docs/reviews/2026-04-20-code-review-report.md @@ -0,0 +1,119 @@ +# 2026-04-20 Code Review Report + +## Scope + +本次 review 以代码实现和真实验证结果为主，不以停用业务说明或文档状态作为主判断依据。重点覆盖： + +- 后端 Go 代码实现与测试执行结果 +- 前端 Vue/TypeScript 实现与质量门禁结果 +- 高风险管理操作的真实代码路径 + +## Validation Summary + +已执行的验证： + +- `backend`: `go test ./...` +- `backend`: 定向复现 `internal/pkg/logger` 相关测试 +- `frontend`: `npm run test:run` +- `frontend`: `npm run typecheck` +- `frontend`: `npm run lint:check` + +验证结论： + +- 后端除 `internal/pkg/logger` 外，其余项目包测试通过 +- 前端 `vitest` 59 个测试文件、354 个测试全部通过 +- 前端 `typecheck` 失败 +- 前端 `lint:check` 失败 + +## Findings + +### 1. High: Windows 下 logger `Sync()` 在标准输出被 pipe 接管时会卡死 + +证据： + +- [logger.go#L199](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L199) +- [logger.go#L274](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L274) +- [logger.go#L275](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L275) +- [logger.go#L404](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger.go#L404) +- [logger_test.go#L12](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L12) +- [logger_test.go#L60](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L60) +- [logger_test.go#L132](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L132) +- [logger_test.go#L169](/D:/project/sub2api-merge/backend/internal/pkg/logger/logger_test.go#L169) + +实际验证中，`go test ./...` 在 `internal/pkg/logger` 失败，`TestInit_DualOutput` 超时 10 分钟，阻塞栈停在 `FlushFileBuffers -> zap Sync -> logger.Sync()`。这说明当前实现对 Windows pipe 场景不安全，不只是测试代码问题。 + +### 2. High: “删除全部未使用兑换码”只删除前 1000 条 + +证据： + +- [RedeemView.vue#L737](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L737) +- [RedeemView.vue#L740](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L740) +- [RedeemView.vue#L749](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L749) +- [RedeemView.vue#L750](/D:/project/sub2api-merge/frontend/src/views/admin/RedeemView.vue#L750) + +当前实现固定只拉取第 1 页、每页 1000 条未使用兑换码，然后直接批量删除并提示成功。当未使用兑换码总数大于 1000 时，剩余数据会被静默遗漏。 + +### 3. Medium-High: 兑换码生成非事务化，叠加幂等重试后存在“部分成功再放大”风险 + +证据： + +- [admin_service.go#L2056](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2056) +- [admin_service.go#L2072](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2072) +- [admin_service.go#L2092](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2092) +- [redeem_handler.go#L103](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L103) +- [idempotency.go#L406](/D:/project/sub2api-merge/backend/internal/service/idempotency.go#L406) +- [idempotency.go#L410](/D:/project/sub2api-merge/backend/internal/service/idempotency.go#L410) + +服务层逐条 `Create`，中间任一点失败都会提前返回；幂等层则把执行错误记为 `failed_retryable`。如果请求已经成功插入部分兑换码，再按同一个 key 重试，存在超量生成的可能。 + +### 4. Medium: 兑换码批量删除会吞掉单条失败，并整体返回成功 + +证据： + +- [admin_service.go#L2104](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2104) +- [admin_service.go#L2107](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2107) +- [admin_service.go#L2111](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L2111) +- [redeem_handler.go#L243](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L243) +- [redeem_handler.go#L252](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L252) +- [redeem_handler.go#L253](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L253) +- [admin_service_delete_test.go#L534](/D:/project/sub2api-merge/backend/internal/service/admin_service_delete_test.go#L534) +- [admin_service_delete_test.go#L543](/D:/project/sub2api-merge/backend/internal/service/admin_service_delete_test.go#L543) + +当前实现只累计成功删除数，失败项不会中断，也不会返回失败明细。更关键的是，测试已经把这种语义固化成预期行为，因此这是实现策略问题，不是单纯的漏测。 + +### 5. Medium: 前端测试全绿，但静态质量门禁仍然损坏 + +证据： + +- [SoraAdminView.vue#L243](/D:/project/sub2api-merge/frontend/src/views/admin/SoraAdminView.vue#L243) +- [SoraAdminView.vue#L259](/D:/project/sub2api-merge/frontend/src/views/admin/SoraAdminView.vue#L259) +- [Icon.vue#L16](/D:/project/sub2api-merge/frontend/src/components/icons/Icon.vue#L16) +- [Icon.vue#L17](/D:/project/sub2api-merge/frontend/src/components/icons/Icon.vue#L17) +- [AppSidebar.spec.ts#L24](/D:/project/sub2api-merge/frontend/src/components/layout/__tests__/AppSidebar.spec.ts#L24) +- [package.json#L8](/D:/project/sub2api-merge/frontend/package.json#L8) +- [package.json#L11](/D:/project/sub2api-merge/frontend/package.json#L11) +- [package.json#L12](/D:/project/sub2api-merge/frontend/package.json#L12) +- [package.json#L14](/D:/project/sub2api-merge/frontend/package.json#L14) + +`vitest` 全绿并不代表前端处于可交付状态。当前 `typecheck` 和 `lint:check` 都失败，说明测试集没有覆盖编译期约束与静态门禁。 + +### 6. Low: 仍有部分管理接口是占位实现 + +证据： + +- [admin_service.go#L765](/D:/project/sub2api-merge/backend/internal/service/admin_service.go#L765) +- [redeem_handler.go#L284](/D:/project/sub2api-merge/backend/internal/handler/admin/redeem_handler.go#L284) +- [proxy_handler.go#L262](/D:/project/sub2api-merge/backend/internal/handler/admin/proxy_handler.go#L262) +- [dashboard_handler.go#L180](/D:/project/sub2api-merge/backend/internal/handler/admin/dashboard_handler.go#L180) + +这批接口目前主要是 mock/占位返回。它们不是这次最关键的问题，但如果继续对外暴露，建议明确收敛策略。 + +## Overall Assessment + +本次代码库的主要问题集中在三类： + +- Windows 环境下的日志稳定性 +- 管理端兑换码批量操作的语义正确性 +- 前端“测试通过但质量门禁失败”的交付断层 + +其中，日志 `Sync()` 卡死和兑换码批量操作缺陷建议优先处理。 diff --git a/docs/reviews/2026-04-20-code-review-task-list.md b/docs/reviews/2026-04-20-code-review-task-list.md new file mode 100644 index 00000000..ae34ce82 --- /dev/null +++ b/docs/reviews/2026-04-20-code-review-task-list.md @@ -0,0 +1,106 @@ +# 2026-04-20 Code Review Task List + +## P0 + +- 修复 Windows 下 `logger.Sync()` 对 `stdout/stderr` pipe 的阻塞问题 +- 为 `internal/pkg/logger` 补充 Windows 兼容回归测试，确保 `go test ./...` 可稳定通过 +- 重新执行后端全量测试，确认 `internal/pkg/logger` 修复后无新增失败 + +## P1 + +- 修复管理端“删除全部未使用兑换码”只处理前 1000 条的问题 +- 明确该功能应采用分页循环删除、后端专用批量接口，还是服务端单点清理命令 +- 为该流程补充前端测试，覆盖“超过 1000 条未使用兑换码”场景 + +- 修复 `GenerateRedeemCodes` 的非事务性问题 +- 明确幂等语义: 部分成功后应回滚、应返回已生成结果，还是应禁止重放生成 +- 为兑换码生成补充失败注入测试，覆盖“部分创建成功 + 重试”路径 + +- 调整 `BatchDeleteRedeemCodes` 返回语义 +- 明确接口是否应该失败即整体失败，或返回 `deleted` / `failed_ids` / `errors` +- 同步更新 handler、前端提示文案和现有测试断言 + +## P2 + +- 修复前端 `typecheck` 失败 +- 处理 [SoraAdminView.vue](/D:/project/sub2api-merge/frontend/src/views/admin/SoraAdminView.vue) 中无效的 `Icon` 名称，或将停用页面排除出编译链 + +- 修复前端 `lint:check` 失败 +- 处理 [AppSidebar.spec.ts](/D:/project/sub2api-merge/frontend/src/components/layout/__tests__/AppSidebar.spec.ts) 中的 `no-regex-spaces` 问题 +- 清理当前 lint warning，避免门禁长期失真 + +## P3 + +- 盘点仍保留 mock 返回的管理接口 +- 对每个接口确定策略: 实现、废弃、隐藏入口，或从路由移除 +- 为仍需保留的接口补充契约测试，避免占位实现长期留在主分支 + +## Recommended Execution Order + +1. 先修复 `internal/pkg/logger`，恢复后端全量测试可信度 +2. 再修复兑换码两个批量操作问题，避免管理端误操作 +3. 然后恢复前端 `typecheck` 和 `lint` +4. 最后清理低优先级占位接口 + +## Assignable Work Packages + +### Backend + +- `BE-LOGGER-001` + 目标: 修复 Windows 下 `logger.Sync()` 对 pipe `stdout/stderr` 的阻塞 + 交付物: `logger.go` 修复、回归测试、`go test ./internal/pkg/logger` 通过 + 验收标准: `TestInit_DualOutput` 和 `TestInit_CallerShouldPointToCallsite` 不再超时 + +- `BE-REDEEM-001` + 目标: 将兑换码生成从“逐条创建 + 部分成功可重试”改为可证明安全的语义 + 交付物: 服务层实现调整、失败注入测试、幂等路径验证 + 验收标准: 不存在“部分成功后同 key 重试超量生成”的路径 + +- `BE-REDEEM-002` + 目标: 重构批量删除返回语义，明确成功、失败和跳过项 + 交付物: 服务层返回结构、handler 响应调整、测试同步 + 验收标准: 部分失败时 API 不再伪装为“完全成功” + +### Frontend + +- `FE-REDEEM-001` + 目标: 修复“删除全部未使用兑换码”只删前 1000 条的问题 + 交付物: `RedeemView.vue` 实现调整、视图测试 + 验收标准: 超过 1000 条未使用兑换码时仍能完整删除或明确失败 + +- `FE-REDEEM-002` + 目标: 对齐新的批量删除 API 语义和用户提示 + 交付物: API 类型、页面提示、异常和部分成功文案 + 验收标准: 用户能看到准确的删除结果 + +- `FE-QUALITY-001` + 目标: 恢复 `typecheck` + 交付物: 修复无效 `Icon` 名称或将停用页移出编译链 + 验收标准: `npm run typecheck` 通过 + +- `FE-QUALITY-002` + 目标: 恢复 `lint:check` + 交付物: 修复 `AppSidebar.spec.ts` 正则问题并清理 warning + 验收标准: `npm run lint:check` 通过 + +### Testing + +- `TEST-BE-001` + 目标: 为 logger Windows 行为建立稳定回归测试 + 交付物: 针对 `Sync()`、stdout/stderr pipe、caller 输出的测试 + 验收标准: 可在 Windows 环境稳定执行，不依赖 10 分钟超时 + +- `TEST-BE-002` + 目标: 为兑换码批量生成/删除建立失败路径覆盖 + 交付物: 服务层和 handler 层测试 + 验收标准: 覆盖部分成功、失败回滚、结果透出 + +- `TEST-FE-001` + 目标: 为 `RedeemView` 补足批量删除流程测试 + 交付物: 视图级测试，覆盖分页循环和部分失败提示 + 验收标准: 批量删除流程不再依赖人工回归 + +- `TEST-FULL-001` + 目标: 恢复全链路质量门禁 + 交付物: 后端 `go test ./...`、前端 `npm run test:run`、`npm run typecheck`、`npm run lint:check` + 验收标准: 四项全部通过 diff --git a/frontend/src/api/admin/redeem.ts b/frontend/src/api/admin/redeem.ts index 57626b1e..df46279f 100644 --- a/frontend/src/api/admin/redeem.ts +++ b/frontend/src/api/admin/redeem.ts @@ -11,6 +11,16 @@ import type { PaginatedResponse } from '@/types' +export interface RedeemBatchDeleteSkipped { + id: number + reason: string +} + +export interface RedeemBatchDeleteResult { + deleted_ids: number[] + skipped: RedeemBatchDeleteSkipped[] +} + /** * List all redeem codes with pagination * @param page - Page number (default: 1) @@ -102,14 +112,8 @@ export async function deleteCode(id: number): Promise<{ message: string }> { * @param ids - Array of redeem code IDs * @returns Success confirmation */ -export async function batchDelete(ids: number[]): Promise<{ - deleted: number - message: string -}> { - const { data } = await apiClient.post<{ - deleted: number - message: string - }>('/admin/redeem-codes/batch-delete', { ids }) +export async function batchDelete(ids: number[]): Promise { + const { data } = await apiClient.post('/admin/redeem-codes/batch-delete', { ids }) return data } diff --git a/frontend/src/components/admin/user/__tests__/UserEditModal.spec.ts b/frontend/src/components/admin/user/__tests__/UserEditModal.spec.ts index 027d24ce..33b5d28f 100644 --- a/frontend/src/components/admin/user/__tests__/UserEditModal.spec.ts +++ b/frontend/src/components/admin/user/__tests__/UserEditModal.spec.ts @@ -119,7 +119,7 @@ describe('UserEditModal', () => { // username 输入框是第二个 text input（第一个是密码） const textInputs = wrapper.findAll('input[type="text"]') // 找到 username 输入框（它有一个 label） - const usernameInput = textInputs.find(input => { + const _usernameInput = textInputs.find(input => { const parent = input.element.closest('div') return parent?.querySelector('label')?.textContent?.includes('admin.users.username') }) || textInputs[1] diff --git a/frontend/src/components/layout/__tests__/AppSidebar.spec.ts b/frontend/src/components/layout/__tests__/AppSidebar.spec.ts index c0da4e38..5f2514ca 100644 --- a/frontend/src/components/layout/__tests__/AppSidebar.spec.ts +++ b/frontend/src/components/layout/__tests__/AppSidebar.spec.ts @@ -21,7 +21,7 @@ describe('AppSidebar custom SVG styles', () => { describe('AppSidebar header styles', () => { it('does not clip the version badge dropdown', () => { - const sidebarHeaderBlockMatch = styleSource.match(/\.sidebar-header\s*\{[\s\S]*?\n \}/) + const sidebarHeaderBlockMatch = styleSource.match(/\.sidebar-header\s*\{[\s\S]*?\n {2}\}/) expect(sidebarHeaderBlockMatch).not.toBeNull() expect(sidebarHeaderBlockMatch?.[0]).not.toContain('@apply overflow-hidden;') diff --git a/frontend/src/i18n/locales/en.ts b/frontend/src/i18n/locales/en.ts index 039d1a01..3a8334f1 100644 --- a/frontend/src/i18n/locales/en.ts +++ b/frontend/src/i18n/locales/en.ts @@ -3195,6 +3195,8 @@ export default { codesExported: 'Codes exported successfully', codeDeleted: 'Redeem code deleted successfully', codesDeleted: 'Successfully deleted {count} unused code(s)', + codesDeletedPartial: 'Deleted {deleted} unused code(s); {failed} could not be deleted', + codesDeleteSkipped: 'No codes were deleted; {failed} code(s) could not be deleted', noUnusedCodes: 'No unused codes to delete', failedToLoad: 'Failed to load redeem codes', failedToGenerate: 'Failed to generate codes', diff --git a/frontend/src/i18n/locales/zh.ts b/frontend/src/i18n/locales/zh.ts index 486e9fd9..852e85f2 100644 --- a/frontend/src/i18n/locales/zh.ts +++ b/frontend/src/i18n/locales/zh.ts @@ -3326,6 +3326,8 @@ export default { codesExported: '兑换码导出成功', codeDeleted: '兑换码删除成功', codesDeleted: '成功删除 {count} 个未使用的兑换码', + codesDeletedPartial: '已删除 {deleted} 个未使用兑换码，还有 {failed} 个删除失败', + codesDeleteSkipped: '未能删除任何兑换码，共 {failed} 个删除失败', noUnusedCodes: '没有未使用的兑换码可删除', userPrefix: '用户 #{id}', failedToExport: '导出兑换码失败', diff --git a/frontend/src/views/admin/RedeemView.vue b/frontend/src/views/admin/RedeemView.vue index fee34f77..8d710559 100644 --- a/frontend/src/views/admin/RedeemView.vue +++ b/frontend/src/views/admin/RedeemView.vue @@ -408,6 +408,7 @@ import { adminAPI } from '@/api/admin' import { formatDateTime } from '@/utils/format' import type { RedeemCode, RedeemCodeType, Group, GroupPlatform, SubscriptionType } from '@/types' import type { Column } from '@/components/common/types' +import { deleteUnusedRedeemCodes } from './redeemDeleteUnused' import AppLayout from '@/components/layout/AppLayout.vue' import TablePageLayout from '@/components/layout/TablePageLayout.vue' import DataTable from '@/components/common/DataTable.vue' @@ -736,18 +737,20 @@ const confirmDelete = async () => { const confirmDeleteUnused = async () => { try { - // Get all unused codes and delete them - const unusedCodesResponse = await adminAPI.redeem.list(1, 1000, { status: 'unused' }) - const unusedCodeIds = unusedCodesResponse.items.map((code) => code.id) + const result = await deleteUnusedRedeemCodes(adminAPI.redeem) + const deleted = result.deletedIds.length + const skipped = result.skipped.length - if (unusedCodeIds.length === 0) { + if (deleted === 0 && skipped === 0) { appStore.showInfo(t('admin.redeem.noUnusedCodes')) - showDeleteUnusedDialog.value = false - return + } else if (skipped === 0) { + appStore.showSuccess(t('admin.redeem.codesDeleted', { count: deleted })) + } else if (deleted > 0) { + appStore.showWarning(t('admin.redeem.codesDeletedPartial', { deleted, failed: skipped })) + } else { + appStore.showInfo(t('admin.redeem.codesDeleteSkipped', { failed: skipped })) } - const result = await adminAPI.redeem.batchDelete(unusedCodeIds) - appStore.showSuccess(t('admin.redeem.codesDeleted', { count: result.deleted })) showDeleteUnusedDialog.value = false loadCodes() } catch (error: any) { diff --git a/frontend/src/views/admin/SoraAdminView.vue b/frontend/src/views/admin/SoraAdminView.vue index 3d107a1e..73791f2a 100644 --- a/frontend/src/views/admin/SoraAdminView.vue +++ b/frontend/src/views/admin/SoraAdminView.vue @@ -240,7 +240,7 @@ onMounted(loadAll)
- +

@@ -256,7 +256,7 @@ onMounted(loadAll)

- +

diff --git a/frontend/src/views/admin/__tests__/SoraAdminView.spec.ts b/frontend/src/views/admin/__tests__/SoraAdminView.spec.ts index 611361f3..f7b0d506 100644 --- a/frontend/src/views/admin/__tests__/SoraAdminView.spec.ts +++ b/frontend/src/views/admin/__tests__/SoraAdminView.spec.ts @@ -53,13 +53,6 @@ const IconStub = defineComponent({ template: '' }) -const BaseButtonStub = defineComponent({ - name: 'BaseButton', - props: ['variant', 'size'], - emits: ['click'], - template: '' -}) - function createMockSystemStats(overrides: Partial = {}): SoraSystemStats { return { total_users: 10, diff --git a/frontend/src/views/admin/__tests__/redeemDeleteUnused.spec.ts b/frontend/src/views/admin/__tests__/redeemDeleteUnused.spec.ts new file mode 100644 index 00000000..c772cdc0 --- /dev/null +++ b/frontend/src/views/admin/__tests__/redeemDeleteUnused.spec.ts @@ -0,0 +1,80 @@ +import { describe, expect, it, vi } from 'vitest' + +import type { PaginatedResponse, RedeemCode } from '@/types' +import { deleteUnusedRedeemCodes } from '../redeemDeleteUnused' + +function createCode(id: number): RedeemCode { + return { + id, + code: `CODE-${id}`, + type: 'balance', + value: 10, + status: 'unused', + used_by: undefined, + used_at: '', + created_at: '2026-01-01T00:00:00Z' + } as RedeemCode +} + +function paginated(items: RedeemCode[]): PaginatedResponse { + return { + items, + total: items.length, + page: 1, + page_size: 1000, + pages: 1 + } +} + +describe('deleteUnusedRedeemCodes', () => { + it('deletes all unused redeem codes across multiple pages', async () => { + const client = { + list: vi.fn() + .mockResolvedValueOnce(paginated([createCode(1), createCode(2)])) + .mockResolvedValueOnce(paginated([createCode(3)])) + .mockResolvedValueOnce(paginated([])), + batchDelete: vi.fn() + .mockResolvedValueOnce({ deleted_ids: [1, 2], skipped: [] }) + .mockResolvedValueOnce({ deleted_ids: [3], skipped: [] }) + } + + const result = await deleteUnusedRedeemCodes(client) + + expect(client.list).toHaveBeenCalledTimes(3) + expect(client.batchDelete).toHaveBeenCalledTimes(2) + expect(client.batchDelete).toHaveBeenNthCalledWith(1, [1, 2]) + expect(client.batchDelete).toHaveBeenNthCalledWith(2, [3]) + expect(result).toEqual({ deletedIds: [1, 2, 3], skipped: [] }) + }) + + it('stops and returns partial results when a batch delete has skipped items', async () => { + const client = { + list: vi.fn().mockResolvedValueOnce(paginated([createCode(1), createCode(2)])), + batchDelete: vi.fn().mockResolvedValueOnce({ + deleted_ids: [1], + skipped: [{ id: 2, reason: 'db error' }] + }) + } + + const result = await deleteUnusedRedeemCodes(client) + + expect(client.list).toHaveBeenCalledTimes(1) + expect(client.batchDelete).toHaveBeenCalledWith([1, 2]) + expect(result).toEqual({ + deletedIds: [1], + skipped: [{ id: 2, reason: 'db error' }] + }) + }) + + it('returns immediately when there are no unused redeem codes', async () => { + const client = { + list: vi.fn().mockResolvedValueOnce(paginated([])), + batchDelete: vi.fn() + } + + const result = await deleteUnusedRedeemCodes(client) + + expect(client.batchDelete).not.toHaveBeenCalled() + expect(result).toEqual({ deletedIds: [], skipped: [] }) + }) +}) diff --git a/frontend/src/views/admin/redeemDeleteUnused.ts b/frontend/src/views/admin/redeemDeleteUnused.ts new file mode 100644 index 00000000..4e0d4c78 --- /dev/null +++ b/frontend/src/views/admin/redeemDeleteUnused.ts @@ -0,0 +1,46 @@ +import type { PaginatedResponse, RedeemCode } from '@/types' +import type { RedeemBatchDeleteResult } from '@/api/admin/redeem' + +type RedeemListClient = ( + page?: number, + pageSize?: number, + filters?: { + status?: 'active' | 'used' | 'expired' | 'unused' + } +) => Promise> + +type RedeemBatchDeleteClient = (ids: number[]) => Promise + +export interface DeleteUnusedRedeemCodesClient { + list: RedeemListClient + batchDelete: RedeemBatchDeleteClient +} + +export interface DeleteUnusedRedeemCodesResult { + deletedIds: number[] + skipped: RedeemBatchDeleteResult['skipped'] +} + +export async function deleteUnusedRedeemCodes( + client: DeleteUnusedRedeemCodesClient, + pageSize: number = 1000 +): Promise { + const deletedIds: number[] = [] + const skipped: RedeemBatchDeleteResult['skipped'] = [] + + for (;;) { + const response = await client.list(1, pageSize, { status: 'unused' }) + const ids = response.items.map((code) => code.id) + if (ids.length === 0) { + return { deletedIds, skipped } + } + + const result = await client.batchDelete(ids) + deletedIds.push(...result.deleted_ids) + skipped.push(...result.skipped) + + if (result.skipped.length > 0) { + return { deletedIds, skipped } + } + } +}