Skip to content

Fix resumed session reactivation#2518

Open
Glucksberg wants to merge 1 commit into
thedotmack:mainfrom
Glucksberg:fix/resumed-session-reactivation
Open

Fix resumed session reactivation#2518
Glucksberg wants to merge 1 commit into
thedotmack:mainfrom
Glucksberg:fix/resumed-session-reactivation

Conversation

@Glucksberg
Copy link
Copy Markdown
Contributor

Summary

Reactivates completed SDK sessions when the same content_session_id receives new work after being resumed.

Changes include:

  • Reads the existing session status when reusing a session by content_session_id.
  • Changes completed resumed sessions back to active.
  • Clears completed_at and completed_at_epoch when a completed session is reactivated.
  • Adds regression coverage in both SessionStore and the standalone session creation helper.

Motivation

A resumed content session can reuse the same SDK session row. If that row remains marked as completed, later processing can treat an active resumed session as finished. Reactivating the row keeps session state consistent with new incoming work.

Validation

This PR is opened as a draft so repository CI can validate it against current upstream. Local review was performed before publishing.

@Glucksberg Glucksberg marked this pull request as ready for review May 17, 2026 18:37
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 17, 2026

Greptile Summary

This PR fixes a state-consistency bug where a resumed SDK session row could remain in completed status after new work arrived for the same content_session_id, causing downstream code to treat an active session as finished. The fix reads status on the existing-row lookup and, when completed, issues a second UPDATE that sets status = 'active', clears the completion timestamps, and resets started_at/started_at_epoch to the current time.

  • Both code paths (SessionStore.ts and sessions/create.ts) are updated symmetrically and include the started_at reset, addressing the duration-inflation concern that applies to the "mark completed again" path.
  • New tests in both test files verify the status flip, timestamp reset, and cleared completion fields with a concrete epoch sentinel value.

Confidence Score: 5/5

The change is safe to merge — it fixes a real state-consistency bug with a narrow, well-tested UPDATE, and both parallel implementations are kept in sync.

The reactivation logic is straightforward: read the status, conditionally issue one UPDATE, return the existing ID. Tests directly validate the status flip, timestamp reset, and cleared completion fields. No existing code path sets sdk_sessions.status = 'failed', so the unhandled terminal status is not a live defect today.

No files require special attention. Both SessionStore.ts and sessions/create.ts are updated symmetrically.

Important Files Changed

Filename Overview
src/services/sqlite/SessionStore.ts Adds status to the SELECT on the existing-session lookup and inserts a reactivation UPDATE when status is completed; the failed status allowed by the schema CHECK constraint is not handled here.
src/services/sqlite/sessions/create.ts Mirror of the SessionStore change — same reactivation block added at the same position; same failed-status gap applies.
tests/session_store.test.ts Adds a regression test that drives markSessionCompleted, backdates started_at, then calls createSDKSession again and asserts status, timestamps, and ID reuse all look correct.
tests/sqlite/sessions.test.ts Adds an equivalent regression test for the standalone createSDKSession helper that manually sets the completed state and checks all fields after reactivation.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant createSDKSession
    participant SQLite

    Caller->>createSDKSession: createSDKSession(contentSessionId, ...)
    createSDKSession->>SQLite: "SELECT id, platform_source, status FROM sdk_sessions WHERE content_session_id = ?"
    SQLite-->>createSDKSession: "{ id, platform_source, status }"

    alt "existing row AND status = 'completed'"
        createSDKSession->>SQLite: "UPDATE sdk_sessions SET status='active', completed_at=NULL, completed_at_epoch=NULL, started_at=now, started_at_epoch=now WHERE id = existing.id"
        createSDKSession-->>Caller: existing.id (reactivated)
    else "existing row AND status != 'completed'"
        createSDKSession-->>Caller: existing.id (unchanged)
    else no existing row
        createSDKSession->>SQLite: INSERT INTO sdk_sessions (...) VALUES (...)
        createSDKSession->>SQLite: "SELECT id FROM sdk_sessions WHERE content_session_id = ?"
        SQLite-->>createSDKSession: "{ id }"
        createSDKSession-->>Caller: new id
    end
Loading
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
src/services/sqlite/sessions/create.ts:64
The reactivation guard checks only for `'completed'`, but the schema's `CHECK` constraint also allows `'failed'` (`CHECK(status IN ('active', 'completed', 'failed'))`). If a session ever enters `'failed'` state, a resumed call with the same `content_session_id` would return the existing ID without reactivating it, leaving the row stuck as `failed` while the caller believes work is proceeding normally. No current code path sets `sdk_sessions.status = 'failed'`, but the guard would silently do nothing if that changes.

```suggestion
    if (existing.status === 'completed' || existing.status === 'failed') {
```

### Issue 2 of 2
src/services/sqlite/SessionStore.ts:1695
Same gap as in `create.ts`: the reactivation check covers only `'completed'` but the schema permits `'failed'` as a third terminal status. A session stuck in `'failed'` would be returned as-is without reactivation if this code path is ever reached with that state.

```suggestion
      if (existing.status === 'completed' || existing.status === 'failed') {
```

Reviews (2): Last reviewed commit: "Fix resumed session reactivation" | Re-trigger Greptile

Comment thread src/services/sqlite/sessions/create.ts Outdated
Comment thread src/services/sqlite/SessionStore.ts Outdated
Glucksberg added a commit to Glucksberg/claude-mem that referenced this pull request May 17, 2026
# Conflicts:
#	tests/session_store.test.ts
@Glucksberg Glucksberg force-pushed the fix/resumed-session-reactivation branch from 1665461 to 8566fef Compare May 17, 2026 20:49
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.

1 participant