Skip to content

fix: preserve codename on re-registration after stale cleanup - #106

Merged
evansenter merged 2 commits into
mainfrom
fix/preserve-codename-on-reregistration
Feb 9, 2026
Merged

fix: preserve codename on re-registration after stale cleanup#106
evansenter merged 2 commits into
mainfrom
fix/preserve-codename-on-reregistration

Conversation

@evansenter

Copy link
Copy Markdown
Owner

Summary

  • When a session's heartbeat exceeded the 24h timeout, cleanup_stale_sessions() would soft-delete it. On re-registration, find_session_by_client() filtered out deleted sessions, causing a new codename to be generated
  • Added include_deleted parameter to find_session_by_client() to recover soft-deleted sessions during registration, reactivating by clearing deleted_at
  • Added 5 tests covering codename preservation, session reactivation, and active-session preference in lookups

Test plan

  • Existing 299 tests pass
  • New regression test: codename preserved after soft-delete + re-register
  • New test: deleted_at cleared on reactivation
  • New storage tests: include_deleted parameter behavior (default excludes, flag includes, prefers active)

Closes #104

🤖 Generated with Claude Code

…leanup

When a session's heartbeat exceeded the 24h timeout, cleanup_stale_sessions()
would soft-delete it. On re-registration, find_session_by_client() filtered
out deleted sessions, causing a new codename to be generated instead of
recovering the original one.

Fix: add include_deleted parameter to find_session_by_client() and use it
during registration to recover soft-deleted sessions. The session is
reactivated by clearing deleted_at.

Closes #104

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@claude

claude Bot commented Feb 7, 2026

Copy link
Copy Markdown

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Fixes codename loss after stale session cleanup by adding include_deleted to find_session_by_client() so that re-registration with the same (machine, client_id) recovers the soft-deleted session's codename and reactivates it. Includes 5 new tests covering the regression scenario and storage-level behavior.

Issues Found

Critical

None

Important

  • server.py:217-220 - Session reactivation does not update cwd or repo. When a session is reactivated, only name, last_heartbeat, and deleted_at are updated on the existing session object before calling add_session(). The cwd and repo values passed to register_session() are returned in the API response (lines 231-232) but the stale values from the original session are persisted to the database. This means list_sessions and other storage queries will show the old cwd/repo, while the caller received the new ones. The fix is to also set existing.cwd = cwd and existing.repo = repo before calling add_session(). (Note: this is a pre-existing issue for the active-session resume path, but this PR widens the window since sessions can now be recovered after potentially much longer periods where cwd is more likely to have changed.)

Suggestions

  • storage.py:430 - The SQL ORDER BY deleted_at IS NOT NULL, last_heartbeat DESC correctly prefers active sessions over deleted ones, but the intent relies on SQLite's boolean-as-integer ordering (0 for NULL/active, 1 for NOT NULL/deleted). A comment explaining this ordering would help future readers.
  • test_storage.py:228-249 - The test_find_session_by_client_include_deleted_prefers_active test creates two sessions with the same (machine, client_id) but different id values. The comment acknowledges "This shouldn't normally happen." Since the (machine, client_id) index is not UNIQUE, this scenario is technically possible but might be worth a brief note on how it could happen (e.g., a race condition or manual DB edit), to make the test's purpose clearer to future readers.

Verdict

REQUEST_CHANGES - One important issue: session reactivation should also update cwd and repo on the persisted session object to avoid a mismatch between the API response and stored state.


Automated review by Claude Code

- Update cwd and repo on the existing session during reactivation to
  keep stored state consistent with the API response
- Add comment explaining SQLite boolean-as-integer ordering in the
  include_deleted query
- Clarify test comment about how duplicate (machine, client_id) entries
  could occur

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@evansenter

Copy link
Copy Markdown
Owner Author

Feedback Addressed

Implemented

  • [Important] server.py:217-220 - Session reactivation now updates cwd and repo on the existing session object before add_session(), keeping stored state consistent with the API response
  • [Suggestion] storage.py:430 - Added inline comment explaining SQLite boolean-as-integer ordering (0 for active, 1 for deleted) in the ORDER BY clause
  • [Suggestion] test_storage.py:232-233 - Updated test comment to explain how duplicate (machine, client_id) entries could occur (race conditions, manual DB edits)

@claude

claude Bot commented Feb 9, 2026

Copy link
Copy Markdown

Prompt: evansenter/dotfiles/.../claude-review.md

Code Review

Summary

Fixes codename loss after stale session cleanup by adding include_deleted to find_session_by_client() so that re-registration recovers soft-deleted sessions. Second round addresses all prior feedback: cwd/repo are now updated on reactivation, SQL ordering is commented, and the edge-case test comment is clarified.

Issues Found

Critical

None

Important

None

Suggestions

None

Previously Addressed (Filtered)

  • server.py:217-220 - Session reactivation now updates cwd and repo (Implemented)
  • storage.py:430 - Added inline comment explaining SQLite boolean-as-integer ordering (Implemented)
  • test_storage.py:232-233 - Updated test comment to explain how duplicate entries could occur (Implemented)

3 items from prior feedback rounds were not re-raised.

Verdict

APPROVE - All prior feedback has been addressed. The fix correctly recovers codenames from soft-deleted sessions, updates all relevant session fields on reactivation, and includes thorough test coverage for the regression and storage-level behavior.


Automated review by Claude Code

@evansenter
evansenter merged commit e05d3a6 into main Feb 9, 2026
5 checks passed
@evansenter
evansenter deleted the fix/preserve-codename-on-reregistration branch February 9, 2026 02:11
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.

Re-registration assigns new codename, breaking cached references

1 participant