fix(server): graceful shutdown on SIGTERM/SIGINT with reset-connected and timeout (#26)

[angelTomo9]

Closes #26

Summary of Changes

Implements graceful server shutdown on SIGTERM and SIGINT to cleanly unmark connected characters in the database and notify connected players before termination.

Features & Robustness

• SIGTERM & SIGINT Signal Handlers: Registers listeners calling gracefulShutdown(signal) with re-entrancy protection.
• Player Notification: Cleanly closes all active client WebSockets with code 1000 and reason "Servidor reiniciando. Por favor vuelve a conectar en unos momentos.".
• Database Reset: Dispatches POST /internal/characters/reset-connected to reset character connection state in PostgreSQL.
• Hard Timeout Fallback: Implements a 5-second unrefed timer to guarantee the process never hangs if API networking is degraded during container destruction.
• Safety Net Retained: Startup reset (resetConnectedCharactersOnStartup) remains active for abrupt crashes.

---

Summary by Gitar

• World Builder & Map Editing:
  • Added map edit permissions and protected map restrictions for major cities in api/src/repositories/worldBuilder.ts
  • Implemented dynamic palette overrides and graphic metadata endpoints in api/src/server.ts

_{This will update automatically on new commits.}

[gitar-bot]

💡 Edge Case: Palette next_id allocation is racy under concurrency

When paletteId is omitted, the next id is computed via SELECT COALESCE(MAX(palette_id),1000)+1 and then inserted in a separate statement. Two concurrent upsertPaletteEntry calls for the same map can compute the same next_id; the ON CONFLICT (map_num, palette_id) DO UPDATE then makes the second silently overwrite the first entry instead of creating a new one. Since map editing is low-frequency admin-only, impact is limited, but consider using a sequence or a single INSERT ... SELECT with a computed id to make allocation atomic.

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

[gitar-bot]

💡 Quality: Account-key fallback references non-existent fields

The accountKey chain checks (user as any).account_id and (client as any).accountId, but only user.idAccount actually exists on the runtime types (RuntimeCharacter.idAccount). The two extra as any conditions are dead code that will never contribute and mask type checking. Since idAccount is optional and undefined for some sessions, those clients silently fall back to socket.getIp(client), re-introducing the CGNAT grouping the PR aims to avoid — worth confirming idAccount is reliably populated. Simplify to user.idAccount ?? socket.getIp(client) ?? idUser.

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

[gitar-bot]

Code Review 👍 Approved with suggestions 1 resolved / 3 findings

Implements graceful server shutdown on SIGTERM and SIGINT with websocket notifications and database connection resets. Consider addressing the racy palette next_id allocation and the non-existent accountKey field references.

💡 Edge Case: Palette next_id allocation is racy under concurrency

📄 api/src/repositories/worldBuilder.ts:619-633

When paletteId is omitted, the next id is computed via SELECT COALESCE(MAX(palette_id),1000)+1 and then inserted in a separate statement. Two concurrent upsertPaletteEntry calls for the same map can compute the same next_id; the ON CONFLICT (map_num, palette_id) DO UPDATE then makes the second silently overwrite the first entry instead of creating a new one. Since map editing is low-frequency admin-only, impact is limited, but consider using a sequence or a single INSERT ... SELECT with a computed id to make allocation atomic.

💡 Quality: Account-key fallback references non-existent fields

📄 server/src/server.ts:782-787

The accountKey chain checks (user as any).account_id and (client as any).accountId, but only user.idAccount actually exists on the runtime types (RuntimeCharacter.idAccount). The two extra as any conditions are dead code that will never contribute and mask type checking. Since idAccount is optional and undefined for some sessions, those clients silently fall back to socket.getIp(client), re-introducing the CGNAT grouping the PR aims to avoid — worth confirming idAccount is reliably populated. Simplify to user.idAccount ?? socket.getIp(client) ?? idUser.

✅ 1 resolved

✅ Edge Case: Race timeout timer not cleared after fetch wins

📄 server/src/server.ts:998-1000 📄 server/src/server.ts:1010
The timeoutPromise schedules a 3.5s setTimeout that is never cleared when fetchPromise wins the Promise.race. The timer is not unref()'d, so it keeps the event loop alive for up to 3.5s after the API call succeeds. It is benign here only because process.exit(0) follows immediately, but it is a latent leak if the code is ever reused. Capture the timer id and clearTimeout it in a finally, or call .unref() on it like the outer forceExitTimeout.

🤖 Prompt for agents

Code Review: Implements graceful server shutdown on SIGTERM and SIGINT with websocket notifications and database connection resets. Consider addressing the racy palette next_id allocation and the non-existent accountKey field references.

1. 💡 Edge Case: Palette next_id allocation is racy under concurrency
   Files: api/src/repositories/worldBuilder.ts:619-633

   When `paletteId` is omitted, the next id is computed via `SELECT COALESCE(MAX(palette_id),1000)+1` and then inserted in a separate statement. Two concurrent `upsertPaletteEntry` calls for the same map can compute the same `next_id`; the `ON CONFLICT (map_num, palette_id) DO UPDATE` then makes the second silently overwrite the first entry instead of creating a new one. Since map editing is low-frequency admin-only, impact is limited, but consider using a sequence or a single INSERT ... SELECT with a computed id to make allocation atomic.

2. 💡 Quality: Account-key fallback references non-existent fields
   Files: server/src/server.ts:782-787

   The `accountKey` chain checks `(user as any).account_id` and `(client as any).accountId`, but only `user.idAccount` actually exists on the runtime types (RuntimeCharacter.idAccount). The two extra `as any` conditions are dead code that will never contribute and mask type checking. Since `idAccount` is optional and undefined for some sessions, those clients silently fall back to `socket.getIp(client)`, re-introducing the CGNAT grouping the PR aims to avoid — worth confirming `idAccount` is reliably populated. Simplify to `user.idAccount ?? socket.getIp(client) ?? idUser`.

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}

[leocagli]

Cierro esta a favor de #130, que es la misma rama con un título que sí describe lo
que el diff hace. Te dejo la medición para que veas de dónde sale.

Las cuatro PRs traen el mismo cambio

#130, #131, #132 y #133 tocan exactamente los mismos 8 archivos, con estos
tamaños:

archivo	#130	#131	#132	#133
api/src/server.ts	+273/-19	+270/-19	+273/-19	+273/-19
api/src/repositories/worldBuilder.ts	+277/-0	+262/-0	+281/-0	+277/-0
server/src/server.ts	+94/-18	+94/-18	+75/-79	+94/-18
worldBuilder_permissions.integration.test.ts	+77/-0	+77/-0	+77/-0	+77/-0
worldBuilder_palette.integration.test.ts	+57/-0	+57/-0	+57/-0	+57/-0
api/src/lib/email.ts	+35/-20	+35/-20	+35/-20	+35/-20
api/schema.sql	+27/-0	+27/-0	+27/-0	+27/-0
api/.env.example	+12/-0	+12/-0	+12/-0	+12/-0

Comparando las líneas cambiadas de cada diff entre sí:

#130 vs #133    562 lineas en comun    100% de #130, 100% de #133
#130 vs #132    540 lineas en comun     96% de #130,  93% de #132
#130 vs #131    524 lineas en comun     93% de #130,  96% de #131

#130 y #133 son el mismo diff, línea por línea. Y las 21 líneas que #131 no
comparte con #130 son código de la paleta del world-builder, que es el tema de
#130 y no el de #131.

Por qué eso es un problema y no un detalle

El título de una PR es lo que dice qué se está revisando. Cuando cuatro títulos
anuncian cuatro arreglos distintos (world-builder, apagado ordenado, penalización
por sesión doble, variables de SES) y los cuatro traen 550 líneas de world-builder,
quien revisa abre esperando una cosa y se encuentra otra. Con 146 PRs abiertas eso
multiplica el trabajo de revisión por cuatro para el mismo cambio.

Además cuentan como cuatro contribuciones cuando son una.

Qué dejo abierto

#130, porque su título describe el bulto real del diff: las 550 líneas de
worldBuilder.ts más api/src/server.ts y los dos tests de world-builder que
todas traen.

Dos cosas sobre #130 antes de que la retomes:

1. Quedó en conflicto. Se mergeó feat(editor): Stage 4 - palette, object/NPC browsers, terrain, and placement #109 (Stage 4 del editor), que reescribió
   worldBuilder.ts y api/src/server.ts en esa misma zona. Hay que rebasarla
   contra main y mirar qué de lo tuyo sigue haciendo falta: puede que parte ya
   esté cubierta.
2. Si el apagado ordenado, la penalización por sesión doble y lo de SES son
   cambios que querés aportar de verdad, van en PRs propias, cada una con su
   diff y nada más. El apagado ordenado ya lo tenés aparte en fix(server): implement graceful shutdown handlers for SIGINT/SIGTERM to reset connected characters (#26) #48, que está limpia
   y mergeable: así es como conviene mandarlas.

main está en verde desde hoy, así que a partir de ahora el CI sirve para decir
si una PR rompe algo. Vale la pena aprovecharlo.