Skip to content

Sessions: archived and deleted sessions stay closed - #676

Merged
alex-clickhouse merged 5 commits into
mainfrom
alex/archived-sessions-final
Nov 10, 2026
Merged

alex-clickhouse merged 5 commits into
mainfrom
alex/archived-sessions-final

Conversation

@alex-clickhouse

@alex-clickhouse alex-clickhouse commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #671.

What

Archived and deleted sessions stay closed:

  • A session in archived leaves that state only through unarchive (with an unarchived event).
  • A run never creates a row for a session id that was deleted.
  • Work that targets an archived or deleted session is refused or cancelled, and the person gets a message.

How

One guard in the engine. run() raises the new SessionClosedError (reason = archived or deleted) for a closed session. It checks once, right after the run holds the session's run lock. The run lock then stays held for the whole turn, and archive and delete refuse a session whose run lock is held. The user message is not stored. A queued entry leaves the queue as refused (the web chat then gives the text back), and the session's clients get an error event that says why. A route that runs a turn answers 409.

Status writes. mark_active, mark_idle, mark_stopped and mark_error write nothing to an archived or missing session (WHERE status != 'archived'), and log their event only when they wrote.

Archive (SessionManager.archive_session):

  • Refuses with SessionBusyError("session is running") while the session has turn work: a turn that runs or waits for a concurrency slot, a registered task (a workflow leg between turns), or anything that holds the run lock (a queued message, a background continuation of the runtime, an idle drain).
  • Right after those checks, with no await in between, it marks the session as closing (SessionManager.closing) until the status is written. A run checks the mark right after it takes the run lock, also with no await in between. So a run either makes the archive fail, or is refused before it stores anything.
  • After the archive: deletes the session's pending wakeups (a "Run later" message included), and expires its pending questions through the normal expiry report (web broadcast, Telegram and Slack card edits). Approvals stay pending: their answer acts on their target, not on the session, and cleanup archives cron sessions.
  • The idle-interactive cleanup no longer expires questions before it archives. If the archive fails, the question stays open.

Channels. _is_within_sticky_period is False for an archived session, so the next channel message starts a new session. If the session is archived after the router resolved it, the router sends the message again once, to a new session.

Delete (DELETE /api/sessions/{id}): answers 409 while the session has turn work, as above. The web chat shows an alert. The delete uses the same check and the same closing mark, so a run cannot start during the delete. After the delete, the session's pending questions expire. The rows stay as history. POST /api/chat with the id of a deleted session creates nothing and answers 409. A plan revision for a deleted proposer session goes to cron:task-planner, as its docstring says.

Answers. handle_answer for an archived or missing session stores the answer and broadcasts it, but starts no turn.

A person's own message reopens. The WebSocket message handler, POST /api/chat with an explicit session_id, "Run later", and a plan revision request for an archived proposer session call engine.reopen_archived. That unarchives the session in that person's name, then the turn runs. Every other caller is refused. Unstarring an archived session keeps it archived and starts no star_project_hook turn.

Why

The state machine audit found five paths that bring an archived or deleted session back as active with no conversation history (SES-1, SES-2, SES-3, NTF-3, NTF-9 in #671). The agent then acted on a wakeup, an answer or a channel message without the context that the person expects, and the chat came back in the sidebar with no unarchived event.

Tests

  • tests/test_closed_sessions.py (26 tests) drives the real engine, run lock, client build and SQLite store with a scripted backend. It includes the three audit repros, changed to assert the expected result: a due wakeup on an archived session fires nothing; a delete with a queued turn answers 409 and a later run creates no row; an answer to a deleted session creates no row.
  • Mutation check: each new guard was removed in turn, and its test failed each time. The guards that the last three commits changed were checked again on the final head.
  • An independent review (Codex) of the first head found one blocker, three majors and two minors. All six are fixed in the second commit, each with a test: the chat API and plan revision created a deleted id again; a turn that started during an archive stored its message before it was refused; background continuations and the queue handoff were not seen by archive and delete; "Run later" into an archived session could never run; unstarring an archived project started a refused turn.
  • The last three commits make the change smaller, with no change in behaviour: one closing mark for archive and delete, one turn-work check, one database lookup for a closed session, one refusal path for a drain and a closed session, and no second check at the client build (no caller could reach it).
  • Two engine doubles get reopen_archived, and the test_drain.py database double returns a session row.
  • Full backend suite on the final head (8855 passed, 39 skipped, equal to the 8894 collected), npm run build, and npm test (141 files, 1911 tests) pass locally.

Not in this PR

🤖 Generated with Claude Code

alex-clickhouse and others added 2 commits November 10, 2026 06:19
An archived session leaves `archived` only through unarchive, and a run
never creates a row for a deleted session id.

- Engine: run() raises SessionClosedError for an archived or deleted
  session. It checks after the run lock and again under the client lock
  while it builds the client. The queue entry leaves as `refused`, and
  the error event says why. Routes answer 409.
- mark_active/idle/stopped/error write nothing to an archived session.
- Archive: refused while a turn holds the run lock (also while it waits
  for a concurrency slot). The check and the status write hold the
  client lock. After the archive, pending wakeups are deleted and
  pending questions expire with the normal reports. Approvals stay.
- Channels: an archived session is not sticky. The router sends a
  message refused by an archive again, once, to a new session.
- Delete: 409 while a turn runs or waits; a run cannot start during the
  delete. The session's questions expire. The web chat shows an alert.
- An answer to an archived or deleted session is stored, no turn.
- A person's own message (web chat, POST /api/chat, plan revision)
  unarchives the session in their name, then runs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Archive: refused while anything holds the session's run lock (a
  queued message, a background continuation, an idle drain) or a
  registered task runs. After the checks, with no await, the session is
  marked as archiving; a run checks the mark right after it takes the
  run lock, so it is refused before it stores anything. This replaces
  the hold of the client lock.
- Delete: also 409 while the run lock is held (the queue handoff).
- POST /api/chat with the id of a deleted session creates nothing and
  answers 409. A plan revision for a deleted proposer session goes to
  cron:task-planner.
- Run later into an archived session unarchives it first.
- Unstarring an archived session starts no star_project_hook turn.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alex-clickhouse
alex-clickhouse marked this pull request as ready for review November 10, 2026 07:02
@nerveai

nerveai commented Nov 10, 2026 •

Copy link
Copy Markdown

Nerve PR review · ✅ Ready to merge

Updated for 9b787f6: three refactor commits with no behaviour change. Archive and delete now share one SessionManager.closing mark and one has_turn_work check, a single closed_session_reason does the DB lookup, and _refuse_turn covers drains and closed sessions alike. The second check at client build is gone. That is safe: every client is built inside _run_inner, which runs only under the run lock (engine.py:4060), and archive and delete refuse a held lock. The verdict stands.

The PR closes the five audited paths (#671) that revived archived or deleted sessions without context. One SessionClosedError gate sits right after the run lock, status writes refuse archived rows in SQL, and the person-facing entry points reopen with an unarchived event. The check-then-mark ordering, with no await between, is sound.

Completeness: 26 tests on the real engine and SQLite; docs/api.md and docs/plans.md updated; backend-agnostic; no migration; merges cleanly into main.
Worth considering (both still apply):

  • router.py:212 retries only without msg.session_id. Telegram reply routing always sets it (telegram.py:2592), so in that race the message is lost. Where the retry does fire, router.py:307 has already shown the error.
  • service.py:595/626: an answer to an archived session counts as answered, but nothing reads it. That includes questions already pending at upgrade. Expiring it instead avoids a silent drop.

Checked: full suite in an offline sandbox at 9b787f6 (3 chunks): 8841 passed, 39 skipped (test_slack_integration.py excluded, hangs offline on main too); CI 4/4 green; frontend not run locally. I read the delta in engine.py, sessions.py, db/sessions.py, the delete and chat routes and plan_service. · automated review by nerveai — mention @nerveai to ask a follow-up.

pufit
pufit previously approved these changes Nov 10, 2026
alex-clickhouse and others added 3 commits November 10, 2026 07:55
- SessionManager.closing(session_id, reason) marks a session while an
  archive or a delete is in progress; closing_reason() reports it. The
  engine's own delete set, begin_delete and end_delete are gone.
- SessionManager.has_turn_work() is the one turn-work check for archive
  and the delete route.
- Database.closed_session_reason() is the one "archived or deleted"
  lookup for the engine, POST /api/chat and the plan revision.
- The race explanation is in one docstring (SessionManager.closing).
- The delete test goes through the delete route, so it also covers the
  route's mark. A unit test covers nested marks and their cleanup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
run() checks the session right after it holds the run lock. From there
the run lock stays held for the whole turn, and archive and delete
refuse a session whose run lock is held, so the second check at the
client build and its except branch in _run_inner had no caller that
could reach them. The test that wrote status='archived' straight to the
database to reach them goes too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_refuse_turn takes the error to raise (default NotAcceptingTurnsError),
so a run in a closed session uses it with SessionClosedError, and the
copy in _refuse_closed_session goes. Every caller that gets a
SessionClosedError already logs it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@alex-clickhouse
alex-clickhouse merged commit 62410a3 into main Nov 10, 2026
4 checks passed
@alex-clickhouse
alex-clickhouse deleted the alex/archived-sessions-final branch November 10, 2026 08:45
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.

Archived and deleted sessions come back to life with no context

3 participants