fix(server): group dual-session idle penalty by account identity for CGNAT mobile support (#16)

[angelTomo9]

Closes #16

Summary of Changes

Fixes accidental disconnections between different mobile players sharing public carrier IPs under Carrier-Grade NAT (CGNAT) by evaluating dual-session scout penalties against account identity rather than public IP address.

Problem & Root Cause

Under CGNAT mobile networks, multiple distinct players share identical external IPv4 addresses. getDuplicateIpIdlePenalizedClientIds previously grouped clients strictly by clientIp, causing active miners on mobile networks to unintentionally trigger 60-second idle scout kicks on unrelated players connected to the same mobile cell tower.

Solution

• Refactored getDuplicateIpIdlePenalizedClientIds to getDuplicateAccountIdlePenalizedClientIds.
• Groups dual-client sessions using authenticated account identity (user.idAccount / user.account_id / client.accountId), preserving anti-scout multi-box protections for single accounts while fully supporting concurrent distinct players on mobile CGNAT networks.

---

Summary by Gitar

• Server Infrastructure & Session Management:
  • Added graceful shutdown handling on SIGTERM and SIGINT signals with a 5-second timeout and /internal/characters/reset-connected API reset.
• World Builder & Map Editing:
  • Implemented protected map restrictions for major capital cities (PROTECTED_MAPS) requiring explicit overrideProtected flags for superadmins and blocking collaborator access.
  • Added database-backed granular map permissions (game_map_permissions) and dynamic map palette overrides (game_map_palette_overrides).
  • Added endpoints for graphic metadata resolution and palette management (/graphics/:grhIndex/metadata, /admin/game-data/maps/:mapNum/palette).

_{This will update automatically on new commits.}

[gitar-bot]

💡 Quality: Dead constant DUPLICATE_IP_IDLE_TIMEOUT_MS after penalty removal

This commit removes the only consumer of DUPLICATE_IP_IDLE_TIMEOUT_MS (the deleted dual-account scout penalty logic), leaving the constant defined but unused. Remove the declaration at line 32 to avoid dead code. Note also that Number(client.lastActivityAt ?? now) at line 726 is now redundant since lines 721-724 already guarantee lastActivityAt is a number; it can be simplified to client.lastActivityAt.

Delete the now-unused constant and simplify the redundant Number(... ?? now) expression.:

// remove line 32:
// const DUPLICATE_IP_IDLE_TIMEOUT_MS = 60 * 1000;

// line 726 simplify:
const idleReferenceAt = client.lastActivityAt;

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review 👍 Approved with suggestions 3 resolved / 4 findings

Groups dual-session idle penalties by account identity to support mobile CGNAT networks and addresses several error handling improvements. Consider removing the now-unused DUPLICATE_IP_IDLE_TIMEOUT_MS constant.

💡 Quality: Dead constant DUPLICATE_IP_IDLE_TIMEOUT_MS after penalty removal

📄 server/src/server.ts:32 📄 server/src/server.ts:726

This commit removes the only consumer of DUPLICATE_IP_IDLE_TIMEOUT_MS (the deleted dual-account scout penalty logic), leaving the constant defined but unused. Remove the declaration at line 32 to avoid dead code. Note also that Number(client.lastActivityAt ?? now) at line 726 is now redundant since lines 721-724 already guarantee lastActivityAt is a number; it can be simplified to client.lastActivityAt.

Delete the now-unused constant and simplify the redundant Number(... ?? now) expression.

// remove line 32:
// const DUPLICATE_IP_IDLE_TIMEOUT_MS = 60 * 1000;

// line 726 simplify:
const idleReferenceAt = client.lastActivityAt;

✅ 3 resolved

✅ Quality: Dead fallback keys account_id and client.accountId never resolve

📄 server/src/server.ts:782-787
The account key resolution (user as any).idAccount || (user as any).account_id || (client as any).accountId || socket.getIp(client) || idUser casts to any and references account_id and accountId, which do not exist anywhere in the codebase — only idAccount (RuntimeCharacter.idAccount) is real. These two fallbacks are dead code and the as any casts silently mask the fact that a future rename of idAccount would fall through to IP grouping unnoticed. Drop the nonexistent fields and use the typed user.idAccount to keep type checking effective.

✅ Quality: ROLLBACK failure can mask the original error

📄 api/src/repositories/worldBuilder.ts:668-670
In the catch block await client.query("ROLLBACK") runs before rethrowing. If ROLLBACK itself rejects (e.g. broken connection), that rejection propagates and swallows the original error, obscuring the real failure cause in logs. Wrap the ROLLBACK in its own try/catch (log-and-ignore) so the original error is always rethrown.

✅ Bug: Account grouping likely neutralizes the anti-scout idle penalty

📄 server/src/server.ts:761-775 📄 server/src/server.ts:703 📄 server/src/server.ts:728-730
getDuplicateAccountIdlePenalizedClientIds now groups sessions by user.idAccount and only penalizes when clientsForAccount.length >= 2 with an active miner. However, standard login calls login.disconnectAllCharacters(account) (server/src/login.ts:508,391), which force-disconnects any existing session sharing the same idAccount before a new one connects — so a single account almost never has two concurrent clients, and the >=2 condition rarely fires. Moreover, real multi-box scouting uses separate accounts sharing one IP, which this account-keyed grouping can no longer detect at all. The net effect is that the dual-session idle penalty is effectively disabled rather than merely made CGNAT-safe. If concurrent same-account sessions are truly impossible, consider removing the now-dead penalty path; otherwise re-evaluate whether IP grouping with an allowlist/opt-out is needed to preserve anti-scout protection while exempting CGNAT.

🤖 Prompt for agents

Code Review: Groups dual-session idle penalties by account identity to support mobile CGNAT networks and addresses several error handling improvements. Consider removing the now-unused DUPLICATE_IP_IDLE_TIMEOUT_MS constant.

1. 💡 Quality: Dead constant DUPLICATE_IP_IDLE_TIMEOUT_MS after penalty removal
   Files: server/src/server.ts:32, server/src/server.ts:726

   This commit removes the only consumer of `DUPLICATE_IP_IDLE_TIMEOUT_MS` (the deleted dual-account scout penalty logic), leaving the constant defined but unused. Remove the declaration at line 32 to avoid dead code. Note also that `Number(client.lastActivityAt ?? now)` at line 726 is now redundant since lines 721-724 already guarantee `lastActivityAt` is a number; it can be simplified to `client.lastActivityAt`.

   Fix (Delete the now-unused constant and simplify the redundant Number(... ?? now) expression.):
   // remove line 32:
   // const DUPLICATE_IP_IDLE_TIMEOUT_MS = 60 * 1000;
   
   // line 726 simplify:
   const idleReferenceAt = client.lastActivityAt;

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

---

Important

Your trial ends in 7 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

_{Was this helpful? React with 👍 / 👎 | Gitar}