mirror of
https://github.com/instructkr/claw-code.git
synced 2026-04-27 01:04:58 +08:00
fix(#160): harden delete_session contract — idempotency, race-safety, typed partial-failure
Addresses review feedback on initial #160 implementation: 1. delete_session() contract now explicit: - Idempotent: delete(x); delete(x) is safe, second call returns False - Race-safe: TOCTOU between exists()/unlink() eliminated via unlink-then-catch - Partial-failure typed: permission/IO errors wrapped in SessionDeleteError (OSError subclass) so callers can distinguish 'not found' (return False) from 'could not delete' (raise) 2. New SessionDeleteError class for partial-failure surfacing. Distinct from SessionNotFoundError (KeyError subclass for missing loads). 3. Caller audit confirmed: no code outside session_store globs .port_sessions or imports DEFAULT_SESSION_DIR. Storage layout is fully encapsulated. 4. Added tests/test_session_store.py — 18 tests covering: - list_sessions: empty/missing/sorted/non-json filter - session_exists: true/false/missing-dir - load_session: SessionNotFoundError typing (KeyError subclass, not FileNotFoundError) - delete_session idempotency: first/second/never-existed calls - delete_session partial-failure: SessionDeleteError wraps OSError - delete_session race-safety: concurrent deletion returns False, not raise - Full save->list->exists->load->delete roundtrip All 18 tests pass. Merge-ready: contract documented, caller-audited, race-safe.
This commit is contained in:
@@ -72,19 +72,50 @@ def session_exists(session_id: str, directory: Path | None = None) -> bool:
|
||||
return (target_dir / f'{session_id}.json').exists()
|
||||
|
||||
|
||||
class SessionDeleteError(OSError):
|
||||
"""Raised when a session file exists but cannot be removed (permission, IO error).
|
||||
|
||||
Distinct from SessionNotFoundError: this means the session was present but
|
||||
deletion failed mid-operation. Callers can retry or escalate.
|
||||
"""
|
||||
pass
|
||||
|
||||
|
||||
def delete_session(session_id: str, directory: Path | None = None) -> bool:
|
||||
"""Delete a session file from the store.
|
||||
|
||||
Contract:
|
||||
- **Idempotent**: `delete_session(x)` followed by `delete_session(x)` is safe.
|
||||
Second call returns False (not found), does not raise.
|
||||
- **Race-safe**: Uses `missing_ok=True` on unlink to avoid TOCTOU between
|
||||
exists-check and unlink. Concurrent deletion by another process is
|
||||
treated as a no-op success (returns False for the losing caller).
|
||||
- **Partial-failure surfaced**: If the file exists but cannot be removed
|
||||
(permission denied, filesystem error, directory instead of file), raises
|
||||
`SessionDeleteError` wrapping the underlying OSError. The session store
|
||||
may be in an inconsistent state; caller should retry or escalate.
|
||||
|
||||
Args:
|
||||
session_id: The session ID to delete.
|
||||
directory: Target session directory. Defaults to DEFAULT_SESSION_DIR.
|
||||
|
||||
Returns:
|
||||
True if the session was deleted, False if it did not exist.
|
||||
True if this call deleted the session file.
|
||||
False if the session did not exist (either never existed or was already deleted).
|
||||
|
||||
Raises:
|
||||
SessionDeleteError: if the session existed but deletion failed.
|
||||
"""
|
||||
target_dir = directory or DEFAULT_SESSION_DIR
|
||||
path = target_dir / f'{session_id}.json'
|
||||
if path.exists():
|
||||
path.unlink()
|
||||
try:
|
||||
# Python 3.8+: missing_ok=True avoids TOCTOU race
|
||||
path.unlink(missing_ok=False)
|
||||
return True
|
||||
return False
|
||||
except FileNotFoundError:
|
||||
# Either never existed or was concurrently deleted — both are no-ops
|
||||
return False
|
||||
except (PermissionError, IsADirectoryError, OSError) as exc:
|
||||
raise SessionDeleteError(
|
||||
f'session {session_id!r} exists in {target_dir} but could not be deleted: {exc}'
|
||||
) from exc
|
||||
|
||||
Reference in New Issue
Block a user