Release 0.90.0

[tudddorrr]

No description provided.

[claude]

Code Review: Release 0.90.0

I've completed a comprehensive review of this PR. Overall, it's well-structured with good test coverage and thoughtful performance optimizations. Here are my findings:

✅ Positive Improvements

1. Excellent refactoring in src/config/scheduled-tasks.ts - the new addScheduledTask helper reduces code duplication
2. Good performance optimization switching from Redis DEL to UNLINK in src/lib/perf/clearCachePattern.ts:14
3. Comprehensive test coverage for new features, especially the stat reset functionality
4. Proper extraction of requestDeleteLeaderboardScore to eliminate code duplication

🚨 Critical Issues to Address

1. Potential Bug - Empty Array Query

File: src/services/game-stat.service.ts:294-298

const aliasIds = aliases.map((alias) => alias.id) await clickhouse.command({ // This could fail if aliasIds is empty query: 'ALTER TABLE player_stats DELETE WHERE stat_id = {statId:Int32} AND player_alias_id IN {aliasIds:Array(Int32)}',

Fix: Add a check for empty aliasIds before executing the ClickHouse query.

2. Transaction Safety Concern

File: src/services/game-stat.service.ts:259-308
The ClickHouse operation (lines 300-305) is inside a MySQL transaction. If ClickHouse fails after MySQL commits, you'll have data inconsistency.
Recommendation: Consider moving ClickHouse operations outside the transaction or implement a two-phase commit pattern.

⚠️ Performance Concerns

1. Complex Query with Large Arrays

File: src/lib/integrations/steamworks-integration.ts:641-657
The $nin: Array.from(syncedPlayerStatIds) with multiple joins could be inefficient for large datasets.
Suggestion: Consider pagination or chunking for large exclusion lists.

2. Data Migration in Schema Migration

File: src/migrations/20250915220838CreateSteamworksPlayerStatTable.ts:13-32
The INSERT statement in the up() migration could be problematic for large datasets.
Recommendation: Consider a separate data migration or batch processing.

🔍 Code Quality Suggestions

1. Complex Function

File: src/lib/integrations/steamworks-integration.ts:714-730
The syncSteamworksStats function orchestrates many operations. Consider breaking it into smaller, focused functions for better maintainability.

2. Silent Error Handling

File: src/lib/integrations/steamworks-integration.ts:575-578

} catch (err) { captureException(err) }

While errors are captured to Sentry, silently continuing might hide important issues. Consider at least logging these errors.

🛡️ Security Review

• ✅ No SQL injection vulnerabilities found - proper use of parameterized queries
• ✅ Authentication/authorization appears intact despite policy simplifications
• ✅ No sensitive data exposure detected

📊 Test Coverage Analysis

Excellent coverage for:

• Stat reset functionality (all modes, edge cases, permissions)
• Steamworks stats synchronization
• Cleanup tasks

Consider adding tests for:

• Error scenarios in syncSteamworksStats
• Concurrent execution of cleanup tasks
• Large dataset performance scenarios

📝 Minor Improvements

1. The return value in ingestSteamworksPlayerStatForAlias (lines 591-616) could be undefined if no stats are processed - consider making this explicit
2. Add database indexes for queries on the new SteamworksPlayerStat entity
3. Consider adding monitoring/metrics for the new scheduled cleanup tasks

Summary

This is a solid release with thoughtful improvements. The critical issues (empty array query and transaction safety) should be addressed before merging. The performance concerns should be monitored in production, and the code quality suggestions can be addressed in follow-up PRs if needed.

Great work on the comprehensive test coverage and the scheduled task refactoring! 🚀

[claude]

Pull Request Review: Release 0.90.0

Thank you for your work on this release. I've conducted a thorough review focusing on code quality, security, performance, and test coverage. Here are my findings:

🔴 Critical Security Issues

1. Steam ID Validation Missing: The Steamworks integration functions (requestDeleteLeaderboardScore, API request handlers) don't validate Steam user ID format, potentially allowing injection attacks through malformed IDs.

2. Parameter Pollution Risk: The querystring.stringify usage with array parameters (e.g., name: [stat.internalName]) could be vulnerable to parameter pollution attacks.

⚠️ Performance Concerns

1. API Rate Limiting Risk:

   • The streamByCursor implementation processes Steam aliases in batches but makes individual API calls per iteration
   • No exponential backoff or proper retry logic in pushPlayerStatsToSteamworks
   • Arbitrary 100ms delay in cleanup tasks may not prevent rate limiting

2. Database Performance:

   • Missing indexes on the new steamworks_player_stat table for frequently queried columns
   • The migration INSERT statement could fail under concurrent execution

🐛 Potential Bugs

1. Race Conditions:

   • Concurrent stat synchronization in ingestSteamworksPlayerStatForAlias could cause data inconsistencies
   • Transaction handling needs improvement with proper rollback mechanisms

2. Data Integrity Issues:

   • ON DELETE SET NULL constraint could create orphaned SteamworksPlayerStat records
   • Missing unique constraint for (stat_id, steam_user_id) combinations

3. Logic Issues:

   • ingestSteamworksPlayerStatForAlias returns early after first stat, potentially missing other stats
   • Silent failures in ingestSteamworksGameStats could lead to incomplete synchronization

📝 Code Quality Improvements

1. Error Handling:

   • Add proper cleanup and transaction rollback on errors
   • Improve error messages for debugging

2. Docker Configuration:

   • The change from bitnami/redis:8.0.3 to redis:8-alpine removes important environment variable handling

3. Scheduled Tasks:

   • Loss of explicit task name constants makes debugging/monitoring harder
   • Consider maintaining named constants for better observability

✅ Positive Aspects

1. Good Test Coverage for:

   • cleanupSteamworksPlayerStats functionality
   • Basic sync scenarios in steamworksSyncStats

2. Performance Improvements:

   • Change from DEL to UNLINK for Redis cache clearing

📋 Recommendations

Before merging, please address:

1. Add Steam ID validation with regex pattern matching

2. Implement proper retry logic with exponential backoff for API calls

3. Add database constraints:

   ALTER TABLE steamworks_player_stat ADD CONSTRAINT unique_stat_steam_user UNIQUE (stat_id, steam_user_id);

4. Fix transaction handling with proper rollback mechanisms

5. Add missing test coverage for:

   • Concurrent access scenarios
   • API rate limiting behavior
   • Database constraint violations
   • Cache invalidation patterns

6. Review Docker changes to ensure Redis compatibility

The PR introduces valuable functionality, but these security and reliability issues should be addressed before production deployment. Happy to discuss any of these points in more detail!