fix(server): use account identity for duplicate-session idle policy (…

[franklincg]

Fixes #16

---

Summary by Gitar

• Connection Policy:
  • Replaced IP-based duplicate session idle penalty with account identity grouping in connectionPolicy.ts
  • Added comprehensive unit tests in testConnectionPolicy.ts covering CGNAT and missing account scenarios

_{This will update automatically on new commits.}

[leocagli]

Te aviso que esta quedo en conflicto con main y necesita un rebase tuyo.

Hoy entraron cuatro merges que tocan esa zona: #109 (los seed que faltaban, npcs.json y objs.json), #80 (apagado ordenado, que reescribio parte de server/src/server.ts), #114 (orden de schema.sql y el ranking) y #121 (los pings como actividad, tambien en server/src/server.ts).

La issue #16 sigue asignada a vos y la PR queda abierta. Cuando rebasees contra main apruebo la corrida del CI y la miramos.

Dos cosas que cambiaron y te conviene saber:

El CI ahora sirve. main estaba en rojo desde el dia que se agrego el workflow, por esos dos archivos de seed que faltaban. Desde hoy esta entero en verde, asi que si el CI de tu rama pasa, quiere decir algo.

Las corridas de los PRs de fork quedan esperando aprobacion. Nadie las estaba aprobando, por eso tu PR nunca mostro resultado. Ya lo estoy destrabando a mano; avisa cuando rebasees y la apruebo.

[gitar-bot]

⚠️ Bug: New npm scripts reference missing files

Both new scripts point at paths that don't exist in the repo: test:client-activity runs src/scripts/testClientActivityPolicy.ts (not present in server/src/scripts/) and test runs tsx --test tests/**/*.test.ts (no server/tests/ directory exists). pnpm test and pnpm test:client-activity will fail immediately, and any CI wired to test will break. Add the missing test file/directory in this PR, or remove the script entries until the tests land.

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

[gitar-bot]

💡 Quality: Test glob may not expand without shell globstar

tsx --test tests/**/*.test.ts relies on ** recursive globbing, which requires shell globstar (bash) or is passed literally to tsx depending on the runner. If the pattern isn't expanded, the test command silently matches nothing or errors. Prefer letting the test runner do the matching (e.g. tsx/node's own glob support) or a portable pattern to avoid environment-dependent behavior.

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

[gitar-bot]

Code Review 🚫 Blocked 0 resolved / 4 findings

Fixes duplicate-session idle policy to use account identity instead of IP-based grouping with added unit tests, but the PR is blocked by four issues: missing test files referenced in npm scripts (testClientActivityPolicy.ts and tests/ directory), a shell-dependent glob pattern in the test script, a missing gracefulShutdown.ts module imported by server.ts that causes startup failure, and a type mismatch on RuntimeClient.close() that doesn't accept the arguments being passed. These must be resolved before merge.

🚨 Bug: Imported ./gracefulShutdown module is missing from this branch

📄 server/src/server.ts:7 📄 server/src/server.ts:211-225

server.ts adds import { createGracefulShutdown } from "./gracefulShutdown" and calls it at module load, but no gracefulShutdown.ts/.js exists in this branch (git ls-tree HEAD server/src shows nothing; the commit 8b28af6 that added it lives only on origin/main and is not an ancestor of this PR). As written the branch fails to compile/resolve the module and the server crashes on startup. Either add gracefulShutdown.ts to this branch or properly merge the main commit that introduces it before merging.

⚠️ Bug: New npm scripts reference missing files

📄 server/package.json:28 📄 server/package.json:35

Both new scripts point at paths that don't exist in the repo: test:client-activity runs src/scripts/testClientActivityPolicy.ts (not present in server/src/scripts/) and test runs tsx --test tests/**/*.test.ts (no server/tests/ directory exists). pnpm test and pnpm test:client-activity will fail immediately, and any CI wired to test will break. Add the missing test file/directory in this PR, or remove the script entries until the tests land.

💡 Quality: Test glob may not expand without shell globstar

📄 server/package.json:28

tsx --test tests/**/*.test.ts relies on ** recursive globbing, which requires shell globstar (bash) or is passed literally to tsx depending on the runner. If the pattern isn't expanded, the test command silently matches nothing or errors. Prefer letting the test runner do the matching (e.g. tsx/node's own glob support) or a portable pattern to avoid environment-dependent behavior.

💡 Quality: client.close called with args but typed as () => void

📄 server/src/server.ts:244

closeClients() calls client.close(1001, SHUTDOWN_CLIENT_MESSAGE), but RuntimeClient.close is declared as close: () => void in types/runtime.ts, so this is a type error under strict compilation even though the underlying WebSocket accepts (code, reason). Widen the RuntimeClient.close signature to (code?: number, reason?: string) => void.

🤖 Prompt for agents

Code Review: Fixes duplicate-session idle policy to use account identity instead of IP-based grouping with added unit tests, but the PR is blocked by four issues: missing test files referenced in npm scripts (`testClientActivityPolicy.ts` and `tests/` directory), a shell-dependent glob pattern in the test script, a missing `gracefulShutdown.ts` module imported by `server.ts` that causes startup failure, and a type mismatch on `RuntimeClient.close()` that doesn't accept the arguments being passed. These must be resolved before merge.

1. ⚠️ Bug: New npm scripts reference missing files
   Files: server/package.json:28, server/package.json:35

   Both new scripts point at paths that don't exist in the repo: `test:client-activity` runs `src/scripts/testClientActivityPolicy.ts` (not present in server/src/scripts/) and `test` runs `tsx --test tests/**/*.test.ts` (no server/tests/ directory exists). `pnpm test` and `pnpm test:client-activity` will fail immediately, and any CI wired to `test` will break. Add the missing test file/directory in this PR, or remove the script entries until the tests land.

2. 💡 Quality: Test glob may not expand without shell globstar
   Files: server/package.json:28

   `tsx --test tests/**/*.test.ts` relies on `**` recursive globbing, which requires shell `globstar` (bash) or is passed literally to tsx depending on the runner. If the pattern isn't expanded, the test command silently matches nothing or errors. Prefer letting the test runner do the matching (e.g. tsx/node's own glob support) or a portable pattern to avoid environment-dependent behavior.

3. 🚨 Bug: Imported ./gracefulShutdown module is missing from this branch
   Files: server/src/server.ts:7, server/src/server.ts:211-225

   server.ts adds `import { createGracefulShutdown } from "./gracefulShutdown"` and calls it at module load, but no gracefulShutdown.ts/.js exists in this branch (git ls-tree HEAD server/src shows nothing; the commit 8b28af6 that added it lives only on origin/main and is not an ancestor of this PR). As written the branch fails to compile/resolve the module and the server crashes on startup. Either add gracefulShutdown.ts to this branch or properly merge the main commit that introduces it before merging.

4. 💡 Quality: client.close called with args but typed as () => void
   Files: server/src/server.ts:244

   closeClients() calls `client.close(1001, SHUTDOWN_CLIENT_MESSAGE)`, but RuntimeClient.close is declared as `close: () => void` in types/runtime.ts, so this is a type error under strict compilation even though the underlying WebSocket accepts (code, reason). Widen the RuntimeClient.close signature to `(code?: number, reason?: string) => void`.

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 4 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more.

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

[franklincg]

Hi @leocagli — I rebased/updated #120 against the current main and resolved the conflict with the recent server changes, including #121.

The CGNAT/account-identity fix for #16 is preserved, and the PR is mergeable again.

Could you approve/run the CI when you have a chance? Thanks.

[franklincg]

Rebase/sync contra `main` já está aplicado no head atual `548c73b`, e as duas verificações desse commit concluíram com sucesso: **CI ✅** e **Secret Scan ✅**. Também conferi os quatro pontos do review contra o head/base atuais: - `server/src/gracefulShutdown.ts` existe no head atual; - `RuntimeClient.close` já aceita `code?` e `reason?`; - `test` e `test:client-activity` já existem iguais no `main` e não foram introduzidos pela #120; - o único script novo desta PR é `test:connection-policy`, e o arquivo correspondente está incluído. Então o review parece ter usado estado anterior/parcial da branch. A PR está mergeable e com CI verde; fica pronta para nova revisão/merge.

…

On Fri, 28 Aug 2026 01:53:44 -0700, "gitar-bot[bot]" ***@***.***> wrote: gitar-bot[bot] left a comment [(Bitcoindefi/OpenAO#120)](#120 (comment)) Reviewing your code Code Review 🚫 Blocked 0 resolved / 4 findings Fixes duplicate-session idle policy to use account identity instead of IP-based grouping with added unit tests, but the PR is blocked by four issues: missing test files referenced in npm scripts (testClientActivityPolicy.ts and tests/ directory), a shell-dependent glob pattern in the test script, a missing gracefulShutdown.ts module imported by server.ts that causes startup failure, and a type mismatch on RuntimeClient.close() that doesn't accept the arguments being passed. These must be resolved before merge. 🚨 Bug: Imported ./gracefulShutdown module is missing from this branch 📄 [server/src/server.ts:7](https://github.com/Bitcoindefi/OpenAO/pull/120/files#diff-4398e3b2ab3e7e7e70a93409fa893abf68608d45a5ad1ac7faaf888dac77005dR7) 📄 [server/src/server.ts:211-225](https://github.com/Bitcoindefi/OpenAO/pull/120/files#diff-4398e3b2ab3e7e7e70a93409fa893abf68608d45a5ad1ac7faaf888dac77005dR211-R225) server.ts adds import { createGracefulShutdown } from "./gracefulShutdown" and calls it at module load, but no gracefulShutdown.ts/.js exists in this branch (git ls-tree HEAD server/src shows nothing; the commit [8b28af6](8b28af6) that added it lives only on origin/main and is not an ancestor of this PR). As written the branch fails to compile/resolve the module and the server crashes on startup. Either add gracefulShutdown.ts to this branch or properly merge the main commit that introduces it before merging. ⚠️ Bug: New npm scripts reference missing files 📄 [server/package.json:28](https://github.com/Bitcoindefi/OpenAO/pull/120/files#diff-da00458cdaeaea2314cb0e0101c85130593048072ada62de01727958c5d6ca37R28) 📄 [server/package.json:35](https://github.com/Bitcoindefi/OpenAO/pull/120/files#diff-da00458cdaeaea2314cb0e0101c85130593048072ada62de01727958c5d6ca37R35) Both new scripts point at paths that don't exist in the repo: test:client-activity runs src/scripts/testClientActivityPolicy.ts (not present in server/src/scripts/) and test runs tsx --test tests/**/*.test.ts (no server/tests/ directory exists). pnpm test and pnpm test:client-activity will fail immediately, and any CI wired to test will break. Add the missing test file/directory in this PR, or remove the script entries until the tests land. 💡 Quality: Test glob may not expand without shell globstar 📄 [server/package.json:28](https://github.com/Bitcoindefi/OpenAO/pull/120/files#diff-da00458cdaeaea2314cb0e0101c85130593048072ada62de01727958c5d6ca37R28) tsx --test tests/**/*.test.ts relies on ** recursive globbing, which requires shell globstar (bash) or is passed literally to tsx depending on the runner. If the pattern isn't expanded, the test command silently matches nothing or errors. Prefer letting the test runner do the matching (e.g. tsx/node's own glob support) or a portable pattern to avoid environment-dependent behavior. 💡 Quality: client.close called with args but typed as () => void 📄 [server/src/server.ts:244](https://github.com/Bitcoindefi/OpenAO/pull/120/files#diff-4398e3b2ab3e7e7e70a93409fa893abf68608d45a5ad1ac7faaf888dac77005dR244) closeClients() calls client.close(1001, SHUTDOWN_CLIENT_MESSAGE), but RuntimeClient.close is declared as close: () => void in types/runtime.ts, so this is a type error under strict compilation even though the underlying WebSocket accepts (code, reason). Widen the RuntimeClient.close signature to (code?: number, reason?: string) => void. 🤖 Prompt for agents Code Review: Fixes duplicate-session idle policy to use account identity instead of IP-based grouping with added unit tests, but the PR is blocked by four issues: missing test files referenced in npm scripts (`testClientActivityPolicy.ts` and `tests/` directory), a shell-dependent glob pattern in the test script, a missing `gracefulShutdown.ts` module imported by `server.ts` that causes startup failure, and a type mismatch on `RuntimeClient.close()` that doesn't accept the arguments being passed. These must be resolved before merge. 1. ⚠️ Bug: New npm scripts reference missing files Files: server/package.json:28, server/package.json:35 Both new scripts point at paths that don't exist in the repo: `test:client-activity` runs `src/scripts/testClientActivityPolicy.ts` (not present in server/src/scripts/) and `test` runs `tsx --test tests/**/*.test.ts` (no server/tests/ directory exists). `pnpm test` and `pnpm test:client-activity` will fail immediately, and any CI wired to `test` will break. Add the missing test file/directory in this PR, or remove the script entries until the tests land. 2. 💡 Quality: Test glob may not expand without shell globstar Files: server/package.json:28 `tsx --test tests/**/*.test.ts` relies on `**` recursive globbing, which requires shell `globstar` (bash) or is passed literally to tsx depending on the runner. If the pattern isn't expanded, the test command silently matches nothing or errors. Prefer letting the test runner do the matching (e.g. tsx/node's own glob support) or a portable pattern to avoid environment-dependent behavior. 3. 🚨 Bug: Imported ./gracefulShutdown module is missing from this branch Files: server/src/server.ts:7, server/src/server.ts:211-225 server.ts adds `import { createGracefulShutdown } from "./gracefulShutdown"` and calls it at module load, but no gracefulShutdown.ts/.js exists in this branch (git ls-tree HEAD server/src shows nothing; the commit 8b28af6 that added it lives only on origin/main and is not an ancestor of this PR). As written the branch fails to compile/resolve the module and the server crashes on startup. Either add gracefulShutdown.ts to this branch or properly merge the main commit that introduces it before merging. 4. 💡 Quality: client.close called with args but typed as () => void Files: server/src/server.ts:244 closeClients() calls `client.close(1001, SHUTDOWN_CLIENT_MESSAGE)`, but RuntimeClient.close is declared as `close: () => void` in types/runtime.ts, so this is a type error under strict compilation even though the underlying WebSocket accepts (code, reason). Widen the RuntimeClient.close signature to `(code?: number, reason?: string) => void`. 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 Your trial ends in 4 days — [upgrade now](https://app.gitar.ai/upgrade?utm_source=github&utm_medium=in_app&utm_campaign=in_app_upgrade&utm_content=footer) to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | — Reply to this email directly, [view it on GitHub](#120?email_source=notifications&email_token=APLHN7PZGP37MLNPWTYJ2TT5MFCBRA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNBVGA2DQMBTHA2KM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5450480384), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/APLHN7PMYKLDTUNVBUVHDZ35MFCBRAVCNFSNUABGKJSXA33TNF2G64TZHMYTGMJWGY4TIMBQGU5US43TOVSTWNJSGMZTOMBVGI3DPILWAI). You are receiving this because you authored the thread.

[leocagli]

Mergeado. Verificado en local con los comandos del CI, no sólo mirando el verde:

pnpm install --frozen-lockfile   rc=0
pnpm run lint                    rc=0
pnpm exec tsc --noEmit           rc=0
pnpm test                        3 pass, 0 fail
pnpm run build                   rc=0
pnpm exec tsx src/scripts/testConnectionPolicy.ts   connection policy tests passed

Lo que me convenció del enfoque es esta línea de connectionPolicy.ts:

// Never fall back to public IP when the authenticated account identity is absent.
if (!accountId) {
    continue;
}

Ahí está el bug entero. Dos personas atrás del mismo CGNAT son dos cuentas distintas, así que dejan de penalizarse entre sí; una persona con dos sesiones de la misma cuenta se sigue penalizando, que es lo que la política quería decir desde el principio. Y sacarlo a su propio módulo lo vuelve testeable sin levantar el servidor.

Gracias por el rebase y por dejar escrito en el comentario qué commit y qué checks, que ahorra tener que ir a buscarlo.