Skip to content

빅챗 스프레드 시트 생성 후 GOGO 이모지를 누른 사람들을 일괄 등록하도록 기능 구현 (#89) - #90

Closed
hepheir wants to merge 5 commits into
mainfrom
hepheir/issue89
Closed

빅챗 스프레드 시트 생성 후 GOGO 이모지를 누른 사람들을 일괄 등록하도록 기능 구현 (#89)#90
hepheir wants to merge 5 commits into
mainfrom
hepheir/issue89

Conversation

@hepheir

@hepheir hepheir commented Aug 1, 2024

Copy link
Copy Markdown
Member

fixes #89

빅챗 시트가 생성되기 이전에 등록을 시도한 사람들에 대하여, 시트 생성 직후 일괄 추가하도록 하였습니다.

!!! 실제 슬렉 환경에서는 테스트 해보지 않았습니다.

@hepheir hepheir changed the title test: 빅챗 스프레드 시트 생성이 완료되기 전 GOGO 이모지를 누른 사람들이 누락되는지 검사하는 테스트 작성 (#89) 빅챗 스프레드 시트 생성 후 GOGO 이모지를 누른 사람들을 일괄 등록하도록 기능 구현 (#89) Aug 1, 2024
Comment on lines +49 to +52
self.gs_client.append_row(worksheet_id, member.transform_for_spreadsheet())
finally:
if error_message:
self.slack_client.send_message(msg=error_message, ts=self.ts)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

사용자가 시트에 추가 된 뒤, 슬렉으로 사용자 정보를 보내주는 부분은 가져오지 못했네요.
해당 부분에 대한 수정이 추가로 필요할 것 같습니다.

@roeniss
roeniss requested review from roeniss and sudosubin and removed request for roeniss and sudosubin August 2, 2024 08:23
@roeniss

roeniss commented Aug 2, 2024

Copy link
Copy Markdown
Contributor

@sudosubin 님이 최종 리뷰어인걸로 하시죠 (한명밖에 안넣어짐..) 저도 리뷰는 하겠습니다

@roeniss roeniss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

동시성이 깔끔하게 해결되지 못한 부분들이 있는것 같은데 코멘트 체크 부탁드립니다 🙇

Comment on lines +101 to +102
def get_emoji(self, channel: str, ts: str, emoji_name: str) -> Optional[Reaction]:
"""channel에 있는 ts 시간에 발송된 메시지에 사용자들이 남긴 반응 목록을 가져온다.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 하면 정확히 "ts 시간에" 남긴 반응만 가져오지 않나요? 밀리세컨드 단위로 찍히는 것 같아서요

Comment on lines +34 to +37
reaction = self.slack_client.get_emoji(
channel=channel,
timestamp=self.ts,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 라인을 지나간 직후에 생성되는 이모지는 어떻게 되는걸까요? 여전히 하나의 처리되지 못한 이모지로 남게 될까요?

roeniss pushed a commit that referenced this pull request Aug 16, 2026
Fixes from the adversarial review of the ported PR #90:

- Duplicate-registration race: the one-shot existing-rows snapshot could
  not close the window where JoinBigchat and the backfill process the
  same user concurrently, and JoinBigchat had no dedupe at all. Both
  paths now go through append_row_if_absent, which serializes the
  check-and-append behind a process-wide lock (the app runs as a single
  gunicorn worker). As a bonus this also absorbs duplicate Slack event
  deliveries.
- A reactor who left the channel (or was deactivated) made the ephemeral
  fan-out crash mid-loop, dropping the remaining users' info messages
  and reporting the whole — already successful — request as failed.
  Ephemeral sends are now per-user best-effort.
- Any other backfill error after the sheet was created also turned into
  the global "try again" error, and retrying the mention cannot succeed
  (same-name worksheet). The backfill now warns in the thread instead.
- get_emoji only caught SlackApiError; socket-level OSErrors (which this
  repo already retries in get_replies) crashed the handler. It now uses
  the same 3-attempt retry and degrades to None.
- N cold member lookups in one backfill each re-read the whole members
  sheet; _fetch_members now has a short (60s) TTL cache.

Test gaps demonstrated by mutation testing are filled: multi-user
batches, the MemberLackInfo branch, the link-before-reactions ordering,
worksheet_id pinning, and a real-SlackClient contract test so the
original PR's kwarg-mismatch bug class cannot silently return.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014iELKuaH7CvNy1GZRd7ptg

roeniss commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

이 PR 소생(revival) 검토 결과

이 PR을 현재 main에 살릴 수 있는지 검토했습니다. 결론: 이대로는 머지 불가, 대신 기능을 현재 구조에 맞게 포팅한 브랜치 claude/pr-90-bigchat-revival-1zsckw를 준비했습니다 (이슈 #89도 다시 열었습니다).

왜 이대로는 머지가 안 되나

  • base(ad4352b, 2024-08) 이후 main에 64개 커밋이 쌓였고, 이 PR이 건드린 6개 파일 전부가 충돌합니다 (mergeable_state: dirty).
  • CreateBigchatSheet는 그 사이 MentionHandler 구조로 리팩터링됐고 시트명 형식 검증(parse_sheet_name)이 추가되어, 이 PR의 diff가 그대로 적용될 수 없습니다.
  • 이 PR의 부수 커밋 2개는 main이 이미 다른 방향으로 정리했습니다: MemberManager 싱글턴화(main은 _get_member_manager() 패턴 유지·확장), strip_multilinetextwrap.dedent(main은 strip_multiline 유지). 포팅에서는 둘 다 제외했습니다.

원본 PR에서 발견된 버그 (포팅하며 수정)

  1. get_emoji 호출부 kwarg 불일치 — 시그니처는 (channel, ts, emoji_name)인데 호출은 get_emoji(channel=..., timestamp=...). 실환경에서 즉시 TypeError가 났을 코드인데, 테스트가 SlackClient를 통째로 mock해서 잡히지 않았습니다. (PR 본문의 "실제 슬랙 환경에서는 테스트 해보지 않았습니다"가 정확히 여기에 해당)
  2. reaction을 읽는 대상 메시지가 잘못됨 — 멘션 댓글(self.ts)의 reaction을 읽는데, 실제로 :gogo:는 모집글(스레드 부모)에 눌립니다. 실환경이었다면 항상 빈 결과가 나와 기능이 조용히 무동작했을 것. 포팅본은 thread_ts(스레드 밖 멘션이면 멘션 글 자체)를 읽습니다.
  3. reaction이 하나도 없는 메시지에서 KeyErrorresponse["message"]["reactions"] 직접 접근. 포팅본은 .get() + 오류 시 None으로 처리합니다.

2024년 리뷰 코멘트 3건 처리

  • @hepheir "등록 정보 안내 메시지 누락" → 등록 정보 ephemeral을 build_registration_info_message()로 추출해 JoinBigchat과 일괄 등록이 공유합니다.
  • @roeniss "ts 시간에 남긴 반응만 가져오는 것 아닌가"reactions.gettimestamp는 시각 필터가 아니라 메시지 식별자라 그 걱정은 해당 없음. 다만 "어느 메시지의 ts인가"가 실제 버그였고(위 2번) 수정했습니다.
  • @roeniss "이 라인 직후에 눌린 이모지는?" → 순서 보장으로 해결: reaction은 시트 링크 메시지를 올린 뒤에 읽습니다. 링크가 올라간 이후의 reaction은 JoinBigchat이 정상 처리하므로, 어느 시점에 눌려도 두 경로 중 한쪽에는 반드시 잡힙니다. 두 경로가 동시에 같은 사람을 처리하는 중복은 아래 append_row_if_absent가 막습니다.

포팅 후 적대적 리뷰 (에이전트 리뷰어 4 + 검증 18, 16건 확정 / 2건 반박) 및 반영

  • 중복 등록 레이스 (major) — 스냅샷 방식 dedupe는 JoinBigchat과의 동시 실행 창을 못 닫고, JoinBigchat 쪽엔 dedupe가 아예 없었음. → GoogleSpreadsheetClient.append_row_if_absent(프로세스 전역 락으로 확인-후-추가 직렬화, 단일 gunicorn worker 전제)를 두 경로 모두 사용. Slack 이벤트 중복 전달로 인한 중복 행도 함께 흡수.
  • ephemeral 발송 실패 전파 (major) — 채널을 떠난/비활성화된 반응자에게 chat.postEphemeraluser_not_in_channel로 실패하면 나머지 인원 안내가 중단되고, 이미 성공한 요청이 "다시 시도해줘" 오류로 보고됨(재시도하면 같은 이름 시트 생성 실패로 또 오류). → ephemeral은 인원별 best-effort, 일괄 등록 전체도 실패 시 스레드 경고로 강등.
  • OSError 미방어 (minor) — 이 저장소가 get_replies에 OSError 3회 재시도를 직접 넣었을 만큼 소켓 오류가 실재하는 환경인데 get_emojiSlackApiError만 잡았음. → 동일한 3회 재시도 후 None 강등.
  • 일괄 등록 시 멤버 시트 N회 읽기 — cold find()가 인원마다 멤버 시트 전체를 다시 읽음. → _fetch_members에 60초 TTL 캐시(신규 멤버 반영 지연을 60초로 제한).
  • 테스트 갭 (뮤테이션 테스트로 실증) — 다중 인원/MemberLackInfo/링크-후-reaction 순서/worksheet_id 미고정, 그리고 mock이 위 1번 버그류를 숨기는 문제. → 시나리오 테스트 추가 + 실제 SlackClient를 끼운 계약 고정 테스트 추가. (72 → 82 tests, all green)
  • 반박된 2건: reactions:read 스코프 부재 시나리오(reaction 이벤트 구독 앱이라 성립 불가), 빈 JOIN_BIGCHAT_EMOJI(안전한 no-op).

알고 있는 한계 (의도적으로 미수정)

  • 같은 스레드에서 "새로운 빅챗"을 두 번 실행하면 부모 글의 기존 반응자들이 새 시트에도 등록됩니다 (운영 실수 케이스, 코드로 구분 불가).
  • 일괄 등록자의 ephemeral에는 캘린더 버튼이 없습니다 (JoinBigchat의 _build_calendar_blocks 공유는 후속 과제로).
  • 일괄 등록 진행 중에 reaction을 취소한 사람은 등록될 수 있습니다 (수 초 수준의 창).
  • 락은 단일 프로세스 전제입니다. gunicorn --workers를 늘리면 무효화되니 그때는 다른 수단이 필요합니다.

브랜치는 push 완료 상태이고, 괜찮으면 이 브랜치로 새 PR을 열어 이 PR을 대체하면 됩니다.


Generated by Claude Code

@roeniss

roeniss commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Let me close this by #129 @hepheir

@roeniss roeniss closed this Aug 16, 2026
roeniss added a commit that referenced this pull request Aug 16, 2026
빅챗 시트 생성 전에 :gogo: 누른 사람들 일괄 등록 — PR #90 소생 (#89)
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