Skip to content

fix: address code review feedback on PvP live session stack#37

Merged
ThunderConch merged 1 commit into
ThunderConch:docs/pvp-contractsfrom
eulneul:docs/pvp-contracts
Apr 12, 2026
Merged

fix: address code review feedback on PvP live session stack#37
ThunderConch merged 1 commit into
ThunderConch:docs/pvp-contractsfrom
eulneul:docs/pvp-contracts

Conversation

@eulneul
Copy link
Copy Markdown
Contributor

@eulneul eulneul commented Apr 12, 2026

코드 리뷰에서 지적된 세 가지를 수정합니다.

변경 사항

sendCurrentSnapshot 직접 변이 수정 (pvp-ws-server.ts)

기존 코드는 파라미터로 받은 session을 직접 변이(nextSeq += 1, updatedAt, eventLog.push)한 뒤 저장했습니다. 코드베이스 전체에서 유일하게 불변성 패턴을 깨는 지점이었고, transport.send()가 예외를 던지면 seq가 이미 증가된 상태로 저장되는 문제가 있었습니다. cloneSession() 후 복사본을 변이하도록 수정했습니다.

seenClientCommandIds O(n) 탐색 주석 추가 (battle-command-service.ts)

배열이 배틀 수명 동안 bounded(turns × seats)되어 있어 선형 탐색이 실용적으로 문제없음을 명시했습니다.

auth token env var 지원 (pvp-live.ts)

--auth-token을 CLI 인자로만 받으면 shell 히스토리에 노출됩니다. PVP_AUTH_TOKEN / PVP_SERVER_URL 환경변수로도 설정할 수 있도록 fallback을 추가했습니다.

🤖 Generated with Claude Code

- sendCurrentSnapshot: clone session before mutation so seq/updatedAt
  are not modified in-place before the transport send; consistent with
  the immutable pattern used everywhere else in PvpWsServer
- battle-command-service: document why seenClientCommandIds linear scan
  is acceptable (bounded by turns × seats, cleared on battle end)
- pvp-live CLI: fall back to PVP_AUTH_TOKEN / PVP_SERVER_URL env vars
  so tokens are not required in shell args (and exposed in history)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ThunderConch
Copy link
Copy Markdown
Owner

Codex Adversarial Review 결과: needs-attention 🚫

Target: branch diff against cf36009 (base) → 27eeccd (head)

PR 설명에 적힌 "transport.send()가 예외를 던지면 seq가 이미 증가된 상태로 저장되는 문제"가 실제로는 해결되지 않았다는 지적입니다.

[high] sendCurrentSnapshot: persist가 transport.send보다 먼저 일어남

src/server/ws/pvp-ws-server.ts:419-431

  • 파라미터 session을 직접 변이하지 않고 cloneSession() 복사본을 쓰도록 바꾼 것까지는 맞음 ✅
  • 하지만 sessionsByRoomIdnextSeq / updatedAt / 새 eventLog를 저장한 뒤에 transport.send(snapshot)을 호출하는 순서는 그대로
  • transport.send()가 동기적으로 throw하면:
    • 클라이언트는 스냅샷을 수신하지 못함
    • 서버 state는 "스냅샷 전송됨"으로 기록됨 (seq 증가 + eventLog 엔트리 추가)
    • 이후 재접속/resume 시 이미 높아진 seq와 존재하지 않는 전송 이력을 기반으로 동작 → 복구/재시도 로직이 깨짐

PR 본문의 주장과 달리 "seq가 먼저 증가된 상태로 저장되는 문제"가 여전히 재현 가능한 상태입니다.

권장 수정

// 현재 흐름 (문제)
sessionsByRoomId.set(roomId, nextSession);
transport.send(snapshot);  // throw 시 상태 오염

// 권장: send 성공 후에만 persist
try {
  transport.send(snapshot);
  sessionsByRoomId.set(roomId, nextSession);
} catch (err) {
  // 원본 session 유지 → 재접속 시 깨끗하게 재시도 가능
  throw err;
}

또는 immutable-update 스타일을 유지하되, nextSession을 미리 빌드해두고 transport.send()가 성공한 이후에만 sessionsByRoomId.set(...)을 호출하는 형태.

Next steps

  • sendCurrentSnapshot — persistence는 snapshot 전송 성공 이후에만 수행하도록 순서 변경
  • transport.send가 throw하도록 강제하는 회귀 테스트 추가 → nextSeqeventLog가 변하지 않음을 assert

🤖 Codex adversarial review via /codex:adversarial-review

@ThunderConch ThunderConch merged commit 25543ac into ThunderConch:docs/pvp-contracts Apr 12, 2026
ThunderConch added a commit that referenced this pull request Apr 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants