Skip to content

Conversation

@simonjiang99
Copy link

Pull Request - Backend | 后端 PR

💡 提示 Tip: 推荐 PR 标题格式 type(scope): description
例如: feat(trader): add new strategy | fix(api): resolve auth issue


📝 Description | 描述

English: Ensure deleting a trader also drops the in-memory instance and resets the competition cache so stale standings don’t linger after removal.
中文: 删除交易员时同步移除内存中的交易员并清空竞赛缓存,避免删除后仍展示过期的竞赛数据。


🎯 Type of Change | 变更类型

  • 🐛 Bug fix | 修复 Bug
  • ✨ New feature | 新功能
  • 💥 Breaking change | 破坏性变更
  • ♻️ Refactoring | 重构
  • ⚡ Performance improvement | 性能优化
  • 🔒 Security fix | 安全修复
  • 🔧 Build/config change | 构建/配置变更

🔗 Related Issues | 相关 Issue

  • N/A

📋 Changes Made | 具体变更

English:中文:

  • api/server.go: 调用 TraderManager.RemoveTrader,无论交易员是否仍在内存中都尝试清理,并确保删除操作也刷新竞赛缓存。
  • manager/trader_manager.go: 新增 RemoveTrader 方法,安全地移除指定交易员并重置 competitionCache 的数据与时间戳,杜绝后续请求命中旧缓存。

🧪 Testing | 测试

Test Environment | 测试环境

  • OS | 操作系统: macOS 15.1 (Apple Silicon)
  • Go Version | Go 版本: go1.25.3
  • Exchange | 交易所: N/A

Manual Testing | 手动测试

  • Tested locally | 本地测试通过
  • Tested on testnet | 测试网测试通过(交易所集成相关)
  • Unit tests pass | 单元测试通过
  • Verified no existing functionality broke | 确认没有破坏现有功能

Test Results | 测试结果

GOCACHE=$(pwd)/.cache/go-build go test ./...

🔒 Security Considerations | 安全考虑

  • N/A (not security-related) | 不适用

⚡ Performance Impact | 性能影响

  • No significant performance impact | 无显著性能影响

If impacted, explain | 如果受影响,请说明:
N/A


✅ Checklist | 检查清单

Code Quality | 代码质量

  • Code follows project style | 代码遵循项目风格
  • Self-review completed | 已完成代码自查
  • Comments added for complex logic | 已添加必要注释
  • Code compiles successfully | 代码编译成功 (go build)
  • Ran go fmt | 已运行 go fmt

Documentation | 文档

  • Updated relevant documentation | 已更新相关文档(PR 模板)
  • Added inline comments where necessary | 已添加必要的代码注释
  • Updated API documentation (if applicable) | 已更新 API 文档

Git

  • Commits follow conventional format | 提交遵循 Conventional Commits 格式
  • Rebased on latest dev branch | 已 rebase 到最新 dev 分支
  • No merge conflicts | 无合并冲突

📚 Additional Notes | 补充说明

English: The cache reset is intentionally broad to guarantee no dangling competition data; follow-ups can refine scope if perf impact surfaces.
中文: 为确保不再有残留竞赛数据,此次清缓存采用全量重置;若后续发现性能需求,可再细化清理策略。


By submitting this PR, I confirm | 提交此 PR,我确认:

  • I have read the Contributing Guidelines | 已阅读贡献指南
  • I agree to the Code of Conduct | 同意行为准则
  • My contribution is licensed under AGPL-3.0 | 贡献遵循 AGPL-3.0 许可证

🌟 Thank you for your contribution! | 感谢你的贡献!

@github-actions
Copy link

github-actions bot commented Nov 7, 2025

🤖 Advisory Check Results

These are advisory checks to help improve code quality. They won't block your PR from being merged.

📋 PR Information

Title Format: ⚠️ Suggestion - Consider using type(scope): description

Recommended format

Valid types: feat, fix, docs, style, refactor, perf, test, chore, ci, security, build

Examples:

  • feat(trader): add new trading strategy
  • fix(api): resolve authentication issue
  • docs: update README

PR Size: 🟢 Small (15 lines: +15 -0)

🔧 Backend Checks

Go Formatting: ✅ Good
Go Vet: ✅ Good
Tests: ✅ Passed

Fix locally:

go fmt ./...      # Format code
go vet ./...      # Check for issues
go test ./...     # Run tests

⚛️ Frontend Checks

Build & Type Check: ✅ Success

Fix locally:

cd web
npm run build  # Test build (includes type checking)

📖 Resources

Questions? Feel free to ask in the comments! 🙏


These checks are advisory and won't block your PR from being merged. This comment is automatically generated from pr-checks-run.yml.

@tangmengqiu
Copy link
Collaborator

Not just remove from the memory, but also clean the trader gorouting

@xqliu
Copy link
Contributor

xqliu commented Nov 8, 2025

代码审查报告 - PR #720

审查结果:⚠️ 需要修复

问题清单

🔴 BLOCKING - 缓存失效策略问题

问题1:清空整个竞赛缓存影响所有交易员

  • 位置manager/trader_manager.go:410-412

  • 问题描述

    • 删除一个 trader 时,清空了整个 competitionCache.data
    • 这会影响所有其他 trader 的竞赛数据缓存
    • 下次查询时需要重新计算所有 trader 的竞赛数据,性能开销大
  • 影响范围

    // 当前实现:删除 trader A 时
    delete(tm.traders, "trader-A")          // ✅ 只删除 A
    tm.competitionCache.data = make(...)    // ❌ 清空所有缓存(包括 B、C、D...)
    
    // 应该:
    delete(tm.traders, "trader-A")          // ✅ 只删除 A
    delete(tm.competitionCache.data, "trader-A")  // ✅ 只删除 A 的缓存

问题2:缓存数据结构不明确

  • 位置manager/trader_manager.go:411
  • 问题描述
    • competitionCache.data 的类型是 map[string]interface{}
    • 不清楚 key 是什么(trader ID? competition ID?)
    • 不清楚 value 的具体结构
    • 建议查看 competitionCache 的定义和使用场景

问题3:并发安全性问题

  • 位置manager/trader_manager.go:405-412
  • 问题描述
    • tm.mutm.competitionCache.mu 是两个不同的锁
    • 删除 trader 和清空缓存之间不是原子操作
    • 可能出现以下竞态:
      // Goroutine 1                     // Goroutine 2
      tm.mu.Lock()
      delete(tm.traders, id)
      tm.mu.Unlock()
                                        // 读取 traders(trader 已删除)
                                        // 更新 competitionCache(包含已删除的 trader)
      tm.competitionCache.mu.Lock()
      tm.competitionCache.data = make(...)
      tm.competitionCache.mu.Unlock()

🟡 技术改进建议

建议1:只删除特定 trader 的缓存

func (tm *TraderManager) RemoveTrader(id string) {
    tm.mu.Lock()
    delete(tm.traders, id)
    tm.mu.Unlock()

    // 只删除该 trader 的缓存,而不是清空所有
    tm.competitionCache.mu.Lock()
    delete(tm.competitionCache.data, id)  // 假设 key 是 trader ID
    // 不要重置 timestamp,其他缓存仍然有效
    tm.competitionCache.mu.Unlock()
}

建议2:如果必须清空所有缓存,添加注释说明原因

// RemoveTrader 从内存中移除指定trader并清空竞赛缓存
// 注意:清空整个缓存是因为竞赛排名依赖所有 trader 的数据,
// 删除一个 trader 会影响其他 trader 的排名,因此需要重新计算
func (tm *TraderManager) RemoveTrader(id string) {
    // ...
}

建议3:考虑使用单个锁保护相关数据

// 如果 traders 和 competitionCache 总是需要同步更新,
// 可以考虑使用同一个锁,或使用更大的临界区:
func (tm *TraderManager) RemoveTrader(id string) {
    tm.mu.Lock()
    defer tm.mu.Unlock()
    
    delete(tm.traders, id)
    
    // 在同一个锁保护下更新缓存
    tm.competitionCache.mu.Lock()
    delete(tm.competitionCache.data, id)
    tm.competitionCache.mu.Unlock()
}

业务层面审查

需求验证:删除 trader 后同步缓存是合理需求
副作用范围:清空所有缓存可能影响系统性能
⚠️ 缓存一致性:需要验证缓存失效策略是否符合业务需求

技术层面审查

性能影响:清空整个缓存导致所有 trader 数据需要重新计算
⚠️ 并发安全:两个锁之间存在竞态窗口
⚠️ 代码清晰度:缓存数据结构和失效策略不够明确

最小修复建议

方案 A:只删除特定 trader 的缓存(推荐)

func (tm *TraderManager) RemoveTrader(id string) {
    tm.mu.Lock()
    delete(tm.traders, id)
    tm.mu.Unlock()

    tm.competitionCache.mu.Lock()
-   tm.competitionCache.data = make(map[string]interface{})
-   tm.competitionCache.timestamp = time.Time{}
+   delete(tm.competitionCache.data, id)
+   // 保持 timestamp,其他 trader 的缓存仍然有效
    tm.competitionCache.mu.Unlock()
}

方案 B:如果确实需要清空所有缓存,添加注释

-// RemoveTrader 从内存中移除指定trader并清空竞赛缓存
+// RemoveTrader 从内存中移除指定trader并清空所有竞赛缓存
+// 注意:由于竞赛排名依赖所有 trader 数据,删除一个 trader 会影响排名,
+// 因此需要清空缓存强制重新计算所有排名
func (tm *TraderManager) RemoveTrader(id string) {
    // ...
}

需要验证的问题

  1. competitionCache.data 的 key 是什么?

    • 如果是 trader ID,应该只删除该 trader 的条目
    • 如果是 competition ID,可能需要清空整个缓存
  2. 删除一个 trader 是否真的需要清空所有缓存?

    • 如果竞赛数据是独立的(每个 trader 的数据互不影响),不需要清空
    • 如果竞赛数据包含排名(依赖所有 trader),可能需要清空
  3. 是否有更好的缓存失效策略?

    • 可以考虑使用"脏标记"而不是直接清空
    • 或者只在查询排名时重新计算

建议的验证步骤

# 1. 查看 competitionCache 的定义
grep -n "competitionCache" manager/trader_manager.go

# 2. 查看缓存如何被使用
grep -n "competitionCache.data\[" manager/

# 3. 查看是否有竞赛排名相关逻辑
grep -n "competition\|ranking" manager/

总结

当前 PR 修复了缓存不同步的问题,但实现方式可能过于激进:

  1. 性能问题:清空所有缓存可能影响系统性能
  2. ⚠️ 设计不明确:不清楚为什么需要清空所有缓存
  3. ⚠️ 并发安全:存在潜在的竞态条件

建议

  • 优先采用方案 A(只删除特定 trader 的缓存)
  • 如果确实需要清空所有缓存,请添加详细注释说明原因
  • 验证缓存数据结构和业务需求,确保失效策略正确

@hzb1115
Copy link
Member

hzb1115 commented Nov 11, 2025

pls fix according to the review suggestions. Thanks

@hzb1115 hzb1115 closed this Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants