fix: destroySession race with concurrent getOrCreateSession (#106)

[fitz123]

Summary

• Closes destroySession can race with concurrent getOrCreateSession during child-exit await #106.
• destroySession now deletes the store entry before awaiting the child-process exit, and calls closeSession(chatId, { persist: false }) so the closing snapshot does not re-add the row.
• Added a race-safe test that injects a slow-exiting child and verifies the store is already empty mid-destroy.

Why

destroySession awaited closeSession, which persisted a fresh snapshot on its way out. During the 0-5s SIGTERM to exit window, a concurrent getOrCreateSession for the same chatId could read that snapshot and spawn with --resume, reviving the session the user just /cleaned. Root cause and reproduction in #106.

Test plan

• npx tsx --test bot/src/tests/session-manager.test.ts -> 60/60 pass
• New test destroySession removes stored state before awaiting child exit (race-safe) fails without the ordering fix
• Full bot test suite in CI

[Copilot]

Pull request overview

Fixes a race where /clean (destroySession) could leave a resumable snapshot in the session store while awaiting a child process exit, allowing a concurrent getOrCreateSession to revive the just-destroyed session.

Changes:

• Added a { persist?: boolean } option to closeSession() and gated the final store write behind it.
• Reordered destroySession() to delete stored state before awaiting child exit, and invoked closeSession(..., { persist: false }) to avoid re-persisting.
• Added a race-focused unit test that injects a slow-exiting child and asserts the store is empty mid-destroy.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
bot/src/session-manager.ts	Reorders destroy vs. close and adds optional persistence to prevent store resurrection during shutdown await.
bot/src/__tests__/session-manager.test.ts	Adds a regression test validating store deletion happens before the child-exit await completes.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The JSDoc says closeSession always persists state, but the new { persist } option means it may intentionally skip persistence. Update the doc comment to reflect the optional persistence behavior (and what persist: false implies) so callers aren’t misled.

Suggested change

	/** Close a session: persist state, SIGTERM child, clean up. */
	/** Close a session: optionally persist state, SIGTERM child, and clean up.
	* By default the final session state is written to the store before teardown.
	* Pass `{ persist: false }` to skip that write when the caller intentionally
	* does not want the final in-memory state persisted (for example, to avoid races).
	*/

[Copilot]

This test injects a fake session directly into manager.active but doesn’t increment sessionsActive; closeSession() will call sessionsActive.dec() and can push the global gauge negative, which may leak state across tests. Consider incrementing the gauge when inserting the fake session (or resetting metrics in this suite) to keep the metric consistent.