본문으로 건너뛰기

Code Review: 9fc5a901 — RawBlock delete()의 TOCTOU 정산 버그 수정

커밋: 9fc5a90154fe5430984bb836e5b3d0f98d3e10be 제목: [Fix][RawBlock] Fix TOCTOU accounting bug in delete() by merging was_indexed into delete_many 작성자: Sangyoon Kwon syk0905.kwon@samsung.com 일자: 2026-05-28

요약​

전반적으로 잘 짜인 정확한 버그 수정. 두 번의 락 획득 사이에 발생할 수 있는 race window를 단일 락으로 합치는 방식이 적절하고, 새 반환 타입 RawBlockDeleteResult는 의미가 명확하며 모든 호출처가 함께 업데이트되었음. 다만 인터페이스/문서/테스트 측면에서 짚을 점이 있음.

정합성 (Correctness) — 통과​

  • put_many 경로(core.py:500–523)에서 키는 _inflight → write → _index로 단조 진행하며, 락 안에서만 상태 전이가 일어남. delete_many도 동일 락에서 _index.pop과 _inflight 취소를 한 번에 처리하므로, was_indexed = (removed_entry is not None)은 삭제 시점의 정확한 상태를 반영함. 커밋 메시지가 주장하는 race condition은 구조적으로 사라짐.
  • 인플라이트 키를 cancel하는 경로(core.py:509–514)에서 put_many는 _index에 추가하지 않고 슬롯을 free로 되돌리므로, _notify_keys_stored가 호출되지 않음. 따라서 was_indexed=False → deleted_size=0이 정합함.
  • 인덱스에 이미 커밋된 경우 _notify_keys_stored가 +slot_bytes 했고, delete가 -slot_bytes 하므로 정합함.

인터페이스/설계​

1) RawBlockDeleteResult 필드별 docstring 부재 (warning)​

[core.py:144–148]에서 클래스 docstring은 한 줄짜리 forward 참조뿐. 필드 의미는 delete_many 쪽에만 적혀 있어, 이 결과를 외부에서 import해 쓰는 다른 호출처에서는 의미를 추적해야 함. 코딩 표준의 "every public function has a complete docstring" 정신을 dataclass에도 적용해 필드 시멘틱을 클래스에 함께 적어주는 게 좋음.

@dataclass(frozen=True)
class RawBlockDeleteResult:
    """Per-key result returned by RawBlockCore.delete_many.

    Attributes:
        deleted: True iff the key existed in either the index or
            inflight set and was actually removed/canceled.
        was_indexed: True iff the key was committed in _index at the
            moment of deletion. Callers use this to decide whether
            to charge slot_bytes back to usage accounting (matching
            the +slot_bytes that _notify_keys_stored credited at commit).
            Valid only when `deleted=True`.
    """
    deleted: bool
    was_indexed: bool

2) "locked & not force" 분기에서 was_indexed=False 반환의 시멘틱 모호 (info)​

[core.py:687–691]에서 잠긴 entry를 스킵할 때 was_indexed=False를 반환함. 이 시점에는 entry is not None(인덱스에 존재)인데도 was_indexed=False로 표기되므로, 필드 이름의 문자 그대로 해석하면 거짓이 됨. 현재 caller는 deleted=False이면 즉시 continue하여 이 값을 보지 않기 때문에 동작상 문제는 없지만, 두 가지 위험이 남음:

  • 향후 누군가 was_indexed를 별도 신호("이미 커밋된 키였는지 여부")로 활용하려 하면 잘못된 결론을 낼 수 있음. 예: 메트릭/로그에서 "lock 때문에 보호된 indexed 키 개수"를 세고 싶을 때.
  • 시멘틱이 "deleted=True일 때만 의미가 있음"으로 암묵적이라, 코드만 보고는 알 수 없음.

해결 방향은 두 가지:

  1. 의미를 좁혀 명시: 필드 1)의 docstring에 "Valid only when deleted=True; otherwise the value is unspecified"라고 못박는다. 변경 비용이 가장 낮음.
  2. 의미를 일관되게 유지: was_indexed를 락 분기에서도 실제 인덱스 존재 여부(entry is not None) 대로 채워 반환한다. caller 동작은 그대로지만 필드 이름과 값이 항상 일치하게 됨.
# 옵션 2 적용 시
if entry is not None and locked and not force:
    results.append(
        RawBlockDeleteResult(deleted=False, was_indexed=True)
    )
    continue

권장은 옵션 1(docstring 보강). 옵션 2는 시멘틱은 더 깔끔해지지만, 락 분기의 was_indexed=True는 "삭제되지 않은 키"에 대한 정보라 caller가 이를 잘못 활용할 가능성이 오히려 새로 생김.

3) existed와 removed_entry/inflight 중복 (info)​

[core.py:683–705]에서 existed를 미리 계산한 뒤, 이후 deleted를 다시 existed and (removed_entry is not None or inflight is not None)로 검사함. 같은 락 안에서 _index.pop과 _inflight.get 사이에 상태를 변경하는 다른 mutator가 없으므로, 다음 두 식은 같은 의미가 됨:

  • existed (= 진입 시점에 index 또는 inflight 에 있었는가)
  • removed_entry is not None or inflight is not None (= pop/get 후 어느 쪽이라도 잡혔는가)

따라서 existed and (...)에서 existed는 redundant함. 단순화 가능:

removed_entry = self._index.pop(encoded_key, None)
inflight = self._inflight.get(encoded_key)
if inflight is not None:
    inflight.canceled = True
self._lock_refcnt.pop(encoded_key, None)
if removed_entry is not None:
    self._append_free_slot_locked(
        self._offset_to_slot(int(removed_entry.offset))
    )
    self._meta_dirty_total += 1
deleted = removed_entry is not None or inflight is not None
results.append(
    RawBlockDeleteResult(
        deleted=deleted,
        was_indexed=removed_entry is not None,
    )
)

단, 이 단순화는 "락이 해제되지 않는 동안 _index와 _inflight만이 키 존재 여부를 결정하며, _lock_refcnt.pop은 키 존재성에 영향을 주지 않는다"는 invariant에 의존함. 현재는 참이지만, 미래에 _lock_refcnt나 다른 자료구조가 키 존재의 또다른 source로 추가될 가능성이 있다면 existed라는 명시적 캡처가 안전망이 됨. 따라서 우선순위는 낮음(info).

테스트​

좋은 점​

  • 두 핵심 경로(was_indexed=True, was_indexed=False)를 결정론적으로 검증함.
  • _write_one을 patch해 inflight 윈도우를 강제하는 접근은 실용적임.

개선 여지​

  • 사적 멤버 의존(warning, mild): adapter._core._write_one을 patch하는 것 ([test_raw_block_l2_adapter.py:634, 641])은 코딩 표준의 "Never access private members of other classes"와 "Tests verify the public interface, not implementation details"에 형식상 어긋남. 다만 inflight 윈도우를 결정론적으로 재현할 다른 수단이 없어 실용적 절충임.
  • force=True 분기 미커버(info): 반환 타입이 바뀌었으므로 lock된 키를 force=True로 지웠을 때의 한 줄 회귀 테스트 정도는 추가해도 좋음.

마이그레이션/호출처 — 통과​

raw_block delete_many 호출처 두 곳이 모두 업데이트됨:

  • [raw_block_l2_adapter.py:508] — result.deleted / result.was_indexed 사용
  • [rust_raw_block_backend.py:342] — [0].deleted로 단일 키 케이스 처리

dax의 delete_many는 별개 코어이므로 영향 없음.

커밋 메시지 — 통과​

What/Why가 명확하고, race 시나리오와 결과(slot_bytes만큼 영구 누수)까지 정량적으로 설명되어 있어 미래에 git blame했을 때도 의도를 즉시 파악할 수 있음.

결론​

Approve, with minor follow-ups (모두 non-blocking):

  1. RawBlockDeleteResult 필드 docstring 보강 + "Valid only when deleted=True" 명시 (warning)
  2. existed 변수 단순화 검토 (info)
  3. force=True 한 줄 회귀 테스트 (info)

핵심 버그 수정과 인터페이스 변경은 정확하고 합당하며, 그대로 머지해도 무방한 수준.