Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoved two Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/game/game.cpp`:
- Around line 11310-11314: The code is treating a null `result` from `SELECT
COUNT(*)` as success; change the `players_online` refresh logic so a null
`result` is treated as a read failure (do not proceed to delete on failed read),
log or propagate the error, and ensure the caller return (the unconditional
`true` at the other location) reflects failure when the read or subsequent
`g_database().executeQuery("DELETE FROM `players_online`;")` fails; specifically
update the block that checks `result`/`getNumber<int>("count")` and the
surrounding flow so `!result` triggers an error path (and the function returns
false or bubbles the error) and only when `count > 0` and the delete query
succeeds do you consider the refresh successful (use the result of
`g_database().executeQuery` to decide success).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 897a4d08-d262-40af-8e4a-33a5990b0356
📒 Files selected for processing (2)
.gitignoresrc/game/game.cpp
beats-dh
left a comment
beats-dh
left a comment
There was a problem hiding this comment.
Code Review
Problema Original
Quando o servidor não tem jogadores online, result->getNumber<int>("count") é chamado sem verificar se result é nulo (possível crash/UB). Além disso, a lambda retornava false via changesMade quando não havia mudanças, fazendo executeWithinTransaction retornar false e gerando spam de [error] Failed to update players online — mesmo sendo um cenário válido.
O null-check adicionado no result é correto e necessário. Porém, existem alguns problemas na abordagem atual:
1. return true incondicional mascara falhas reais
A remoção de changesMade e o return true incondicional fazem com que qualquer falha de DB (ex: stmt.execute(), executeQuery()) seja ignorada — a transação é commitada e reportada como sucesso mesmo sem ter feito nada.
O problema original era apenas que changesMade ficava false no cenário legítimo de "nenhum jogador online e nenhum registro stale". A correção deveria distinguir entre "nada a fazer (sucesso)" e "operação falhou (erro)".
Sugestão para o branch m_players.empty():
if (m_players.empty()) {
auto result = g_database().storeQuery("SELECT COUNT(*) AS count FROM players_online;");
if (!result) {
return false; // Query falhou de verdade
}
int count = result->getNumber<int>("count");
if (count > 0) {
return g_database().executeQuery("DELETE FROM `players_online`;");
}
return true; // Nenhum registro stale — sucesso legítimo
}E para o branch else, verificar retornos das operações de DB:
if (!stmt.execute()) {
return false;
}
// ...
if (!g_database().executeQuery(cleanupQuery.str())) {
return false;
}
return true;2. Resultado nulo de storeQuery ignorado silenciosamente
Quando result é nullptr (query falhou), o código atual simplesmente pula o bloco e retorna true. Isso significa que dados stale em players_online nunca seriam limpos, e o sistema reportaria sucesso. Deveria retornar false para sinalizar a falha.
3. .gitignore fora de escopo
As adições de canary-debug e .vscode/launch.json são mudanças de ambiente local que não têm relação com o fix de players online. Recomendo separar em outro commit/PR para manter o histórico limpo.
Resumo
| Aspecto | Avaliação |
|---|---|
Null-check no result |
✅ Correto e necessário |
return true incondicional |
❌ Mascara falhas reais de DB |
| Falha de query silenciosa | false |
.gitignore no mesmo PR |
O PR resolve o sintoma (log spam) mas a abordagem de return true incondicional troca um problema visível (logs) por um invisível (falhas silenciosas). Sugiro retornar true apenas nos cenários legitimamente "sem ação necessária" e propagar false quando operações de DB falharem.
Generated by Claude Code
|
Resolved conflict in .gitignore: kept HEAD's /plan, /enc_temp_folder, and staticdatahouse.dat entries which PR opentibiabr#3904 predated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 participants
Description
This PR fixes a log spam irregularity in the
Game::updatePlayersOnlinefunction where the server would repeatedly complain about failing to update the players online when the player count was exactly 0.The
executeWithinTransactionlambda was hittingfalseinside the iteration block because there were no players to map/update, throwing an unintended[error]continuously onto the server logs. Added a safety condition allowing it to exit returningtruegracefully if no operation was strictly needed, alongside an extra safety check for validating the Database result before fetchinggetNumber<int>.Behaviour
Actual
When having an empty test server or exactly 0 players online, the server console keeps periodically spamming:
[error] [Game::updatePlayersOnline] Failed to update players online.Expected
The server skips updating the database and fails gracefully when the player pool is empty without spamming log errors, recognizing that returning
trueinside an empty operation pool is a valid transaction scenario.Fixes # (if there's an issue number, add it here, otherwise you can leave blank or delete this line)
Type of change
How Has This Been Tested
The server was natively compiled and executed on a Kubuntu Desktop system. Stayed running in the idle loop with a clean and loaded OTBM database configuration without establishing any client connections. The log spam ceased completely while the server ran flawlessly.
Test Configuration:
Checklist
Summary by CodeRabbit
Bug Fixes
Chores