버그를 고치면서 새로 연 문 — 리뷰가 아니었으면 그대로 배포됐다

랜챗 신고 기능을 고치면서 끝난 페어를 최근 3개까지 서버가 기억하게 만들었다. 상대가 통화 중간에 이탈해버리면 신고할 대상 정보 자체가 서버에서 사라지던 것을, 최근에 끝난 페어를 잠깐 보관해뒀다가 참조하는 방식으로 바꾼 것이다. 상대가 이탈한 뒤에도 신고할 수 있게 하기 위해서였다. 테스트를 다 붙였고 전부 통과했다. 코드 리뷰를 올렸다.

리뷰가 이걸 찾았다. 코드 자체는 새로 추가된 부분만 놓고 보면 각각 그럴듯했다. 신고 처리 로직도, 통화 종료 처리 로직도, 최근 페어를 기억하는 로직도 개별적으로는 자연스러운 코드였다. 문제는 그 셋을 하나의 요청 흐름 안에 같이 뒀을 때 서로 어떤 순서로 상호작용하는지였고, 그건 각 로직을 따로 읽어서는 보이지 않았다.

새로 열린 구멍

신고가 성공해도 소비한 레코드를 지우지 않았다. 그리고 현재 페어를 신고하면 통화가 종료되는데, 그 종료 처리가 방금 끝난 그 페어를 "최근에 끝난 페어" 보관함에 다시 넣는다. 신고 처리와 통화 종료 처리가 같은 요청 안에서 순서대로 일어나다 보니, 신고 자체가 스스로 자기가 참조할 수 있는 새 레코드를 만들어내는 구조가 된 것이다. 두 사실이 합쳐지면 다음과 같은 흐름이 가능해진다.

• A가 현재 상대 B를 신고한다 → 기록됨 + 통화 종료 → 그 페어가 보관함에 들어간다
• A가 같은 페이로드를 다시 보낸다 → 이번엔 현재 페어 경로가 아니라 "끝난 페어" 경로로 통과 → 또 기록된다
• 그 레코드가 다시 삭제되지 않으니, 보관함에서 밀려나거나 접속이 끊길 때까지 이 과정을 반복할 수 있다

기존 코드에서는 애초에 불가능했던 일이다. 예전에는 신고 한 번이 끝나면 서버가 들고 있던 상대 정보 자체가 비었고, 그러면 두 번째 신고 시도는 참조할 대상이 없어 그냥 거부됐다. 즉 이건 새 기능이 있어서 생긴, 우리 변경이 새로 열어준 구멍이었다. 예전 코드에는 존재하지 않던 공격 표면이 기능 확장과 함께 같이 태어난 셈이다.

더 정확히 말하면, 최근 3개까지 페어를 기억하도록 늘린 그 순간부터 "신고 대상 정보가 살아있는 기간"과 "신고를 처리할 수 있는 기간"이 더 이상 일대일로 묶이지 않게 됐다. 예전에는 이 둘이 정확히 같은 수명을 가졌기 때문에 중복 신고라는 개념 자체가 성립할 수 없었다. 기억하는 범위를 늘리는 순간 그 전제가 깨졌는데, 구현할 때는 "신고를 더 넓은 범위에서 받을 수 있게 한다"는 목표에만 집중했지 "한 번 처리된 신고를 어떻게 소비 완료 상태로 만들 것인가"는 별도로 생각하지 않았다.

차단 기록 쪽은 같은 대상에 대한 중복 삽입을 막는 제약이 걸려 있어서 이 흐름을 타도 무해했다. 하지만 신고 테이블에는 그런 제약이 없었고, 같은 대상에 대한 행이 원하는 만큼 계속 쌓일 수 있었다. 신고 한 건이 여러 건으로 부풀려지면 모더레이션 쪽에서 보는 신고 집계 수치 자체가 왜곡된다. 실제로 문제가 몇 건인지보다 누가 이 구멍을 몇 번 반복했는지가 수치에 섞여 들어간다.

"클라이언트 UI는 신고 후 모달을 닫으니 같은 요청을 두 번 보낼 일이 없지 않나"는 방어가 안 된다. 이 서버 파일은 클라이언트가 조작될 수 있다는 것을 전제로 짜여 있었고, 그 전제는 코드 안 주석에도 스스로 적혀 있었다. 정상적인 UI 흐름을 가정한 방어는 이 코드의 설계 원칙과 맞지 않는 논리였다.

수정

한 페어당 신고는 딱 한 번만 처리되도록 하고, 성공한 신고가 소비한 레코드는 처리 즉시 지우게 했다. 여기서 순서가 중요했다 — 통화 종료 처리가 그 페어를 "최근에 끝난 페어" 보관함에 새로 밀어 넣기 때문에, 삭제는 반드시 그 종료 처리 뒤에 해야 한다. 삭제를 앞에 두면, 지운 직후에 종료 처리가 같은 레코드를 다시 채워 넣어 삭제한 의미가 없어진다. 정리 코드와 생성 코드가 한 요청 흐름 안에 같이 있을 때는 무엇을 먼저 지우고 나중에 지우느냐가 곧 정확성의 전부였다.

회귀 테스트는 똑같은 신고 페이로드를 연달아 두 번 보내고, 기록이 정확히 한 번만 일어나는지를 확인한다. 수정 전 코드로 돌려 실행하면 이 테스트가 실패하는 것도 함께 확인했다. 이 확인 자체가 이번 사고에서 배운 것이었다 — 테스트를 새로 쓸 때는 통과하는 것만 보지 말고, 고치기 전 코드에 그 테스트를 붙여서 실제로 떨어지는지까지 봐야 그 테스트가 진짜로 이 버그를 잡아내는 테스트인지 확인할 수 있다.

같은 리뷰가 찾은 두 번째 것 — 하중 없는 테스트

같은 브랜치에 이런 테스트가 이미 있었다: "이탈한 상대를 차단할 때, 양쪽 사용자 행 생성을 모두 기다린 뒤에 저장한다."

이 성질은 실제로 중요하다. 기다리지 않고 저장을 시도하면 외래키 위반으로 차단 자체가 조용히 유실될 수 있다. 그러면 사용자는 분명히 차단 버튼을 눌렀고 화면도 성공으로 보였는데, 정작 DB에는 아무것도 안 남아서 재접속하면 방금 차단한 사람과 다시 매칭되는 일이 생긴다. 사용자 신뢰와 직결되는 문제라 테스트를 붙여둔 것 자체는 맞는 판단이었다.

그런데 그 테스트는 실제로는 아무것도 지키지 않고 있었다. 신고자는 이미 접속 중인 사용자라 자기 행 생성이 그 시점에 이미 진행 중이었고, 그것 하나만으로 첫 번째 단언이 성립해버렸다. 그래서 코드를 "신고자 쪽 promise만 기다리고 떠난 상대 쪽은 기다리지 않음"으로 줄여도, 심지어 이번 커밋이 고치기 전의 옛 코드로 통째로 되돌려도 이 테스트는 그대로 통과했다. 정작 이번 커밋이 새로 고친 부분 — 떠난 상대 쪽 행 생성까지 기다리는 것 — 은 이 테스트에서 단 한 번도 검증되지 않고 있었다.

고친 테스트는 두 사용자의 행 생성 promise를 각각 호출 인자로 식별해서 따로따로 풀 수 있게 만들었다. 신고자 쪽 promise만 풀었을 때는 아직 저장이 일어나지 않는 것을, 떠난 상대 쪽 promise까지 풀어야 비로소 저장이 일어나는 것을 순서대로 확인한다. 이렇게 바꾸자 "신고자 쪽만 기다림"으로 줄인 코드에서 실제로 테스트가 빨간불이 됐다.

이 두 번째 사례가 첫 번째 사례보다 더 씁쓸했다. 첫 번째는 새 기능을 추가하면서 새로 생긴 결함이니 "확장할 때는 더 넓게 생각하자"는 교훈으로 정리가 된다. 두 번째는 다르다. 지켜야 할 성질을 정확히 알고 있었고, 그 성질을 테스트로 남기겠다는 판단도 옳았다. 문제는 그 테스트가 실제로 그 성질을 검증하는 코드로 이어지지 못했다는 것뿐이다. 의도와 구현 사이에 벌어진 틈이었고, 리뷰가 없었다면 그 틈은 테스트 통과율 숫자 뒤에 영원히 숨어 있었을 것이다.

교훈

• 기능을 확장하면 예전에는 물리적으로 불가능했던 조합이 새로 가능해진다. "이건 원래 안 되던 조합인데" 하고 지나쳤던 자리가 곧 새로 열린 공격 표면이다. 무엇이 새로 가능해졌는지를 기능 구현이 끝난 뒤에도 따로 물어봐야 한다.
• 정리 코드와 생성 코드가 같은 요청 흐름 안에 있으면 순서가 곧 정확성이다. 지우는 코드가 만드는 코드보다 먼저 실행되면, 지운 것이 곧바로 다시 채워져 아무 효과가 없다.
• 하중 없는 테스트는 통과할 때가 가장 위험하다. 테스트가 존재한다는 사실 자체가 그 성질이 지켜지고 있다는 착각을 준다. 구현을 일부러 되돌려서 실제로 빨간불이 뜨는지 확인하지 않으면, 테스트가 있다는 사실이 오히려 검증을 대체해버리고 만다.