fix: persist Teams chat poll cursor through MemoryBackend (#17)#18
Merged
Conversation
Background Teams poll's per-chat cursor (last_ts, seen_ids, bootstrapped) used to live in-process only at _state["watched_chats"][chat_id]. On every MCP restart the bootstrap path re-fired "newest at boot" as if fresh — even when that message was days old — and messages that arrived during a server-down window were silently dropped. This change persists the cursor through the same MemoryBackend protocol used by interaction_log / daily_summary (ADR-005 Phase 2), parallel to the email_poll cursor pattern. One key per chat (chat_cursors/<id>.json) so a busy chat doesn't rewrite a giant blob. 24h staleness cap on last_ts re-baselines genuinely old cursors instead of replaying stale messages. Debounced ~1s async save coalesces bursts in chatty chats; graceful shutdown flushes dirty cursors so the next process inherits the latest watermark. - New module src/entrabot/tools/chat_cursors.py (load_cursor / save_cursor / is_stale / bound_seen_ids). - _register_watched_chat rehydrates fresh cursors and skips _bootstrap_chat; stale/absent/corrupt cursors fall through to the existing bootstrap path (first-time semantics preserved). - _background_poll schedules a debounced save after every cycle that advances last_ts or seen_ids. - _bootstrap_chat persists its watermark so a restart doesn't re-baseline from "newest at boot." - Lifespan shutdown calls _flush_chat_cursors() in the finally block. - seen_ids_tail bounded to ~50 most recent IDs on serialize. +35 tests (25 module-level + 10 wiring). 1278 passing, ruff clean. Refs #17. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #17 by persisting the per-chat Teams background-poll cursor (last_ts, seen_ids_tail, bootstrapped) via the existing MemoryBackend, so MCP restarts don’t replay old “newest-at-boot” messages or drop messages that arrive while the server is down.
Changes:
- Adds
src/entrabot/tools/chat_cursors.pyto load/save per-chat cursor files atchat_cursors/<chat_id>.json, with a 24h staleness policy and boundedseen_ids_tail. - Wires cursor rehydration + debounced persistence + shutdown flush into
src/entrabot/mcp_server.py. - Adds targeted tests for cursor behavior and MCP server wiring, and updates engineering status/test count.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/entrabot/tools/chat_cursors.py |
New cursor persistence helpers (keying, staleness check, bounded seen-id tail). |
src/entrabot/mcp_server.py |
Rehydrate cursor on chat registration; debounce saves during polling; flush dirty cursors on shutdown. |
tests/tools/test_chat_cursors.py |
Unit tests for cursor keying, bounding, staleness, and round-trip persistence. |
tests/test_mcp_server_chat_cursors.py |
Integration-style tests validating MCP cursor rehydration, debounced saves, and flush behavior. |
docs/engineering-status.md |
Updates “Recently Shipped” status and test count. |
Comment on lines
+603
to
+614
| def _serialize_chat_state(chat_state: dict) -> dict: | ||
| """Convert an in-memory ``chat_state`` dict into the on-disk cursor shape. | ||
|
|
||
| ``seen_ids`` is a set in memory; ``seen_ids_tail`` is a list on disk so it | ||
| serializes deterministically across MemoryBackend implementations. | ||
| """ | ||
| seen = chat_state.get("seen_ids") or set() | ||
| return { | ||
| "last_ts": chat_state.get("last_ts"), | ||
| "seen_ids_tail": list(seen), | ||
| "bootstrapped": bool(chat_state.get("bootstrapped", False)), | ||
| } |
Comment on lines
+693
to
+700
| try: | ||
| chat_state = (_state.get("watched_chats") or {}).get(chat_id) | ||
| if chat_state is not None: | ||
| _chat_cursor_save(chat_id, _serialize_chat_state(chat_state)) | ||
| except Exception as exc: # noqa: BLE001 | ||
| if logger: | ||
| logger.warning("Sync cursor save failed for %s: %s", chat_id, exc) | ||
| return |
Comment on lines
+221
to
+227
| def test_at_cap_boundary_is_stale(self) -> None: | ||
| # Exactly at the cap — treat as stale; better to bootstrap than to | ||
| # surface a borderline-old message as live. | ||
| at_cap = ( | ||
| datetime.now(UTC) - timedelta(seconds=CURSOR_STALENESS_SECONDS + 1) | ||
| ).strftime("%Y-%m-%dT%H:%M:%SZ") | ||
| assert is_stale(at_cap) is True |
Comment on lines
+125
to
+128
| Best-effort: backend write failures are logged and re-raised. Call sites | ||
| inside the poll loop must wrap this in try/except so a single bad write | ||
| doesn't take down the loop. (See ``mcp_server._schedule_cursor_save``.) | ||
| """ |
…t cleanup, test name, docstring) Four review comments on PR #18 (#18): 1. (MEDIUM) `_serialize_chat_state()` did `list(set)` which is non-deterministic across runs — the `[-50:]` tail could drop the actual most-recent IDs the overlap-window dedupe needs after restart. Sort lexicographically and slice; Teams message IDs are numeric-as-string so lex order is monotonic with sent-at. Matches the existing `sorted(...)[-100:]` heuristic at mcp_server.py:1301. 2. (LOW) `_schedule_cursor_save()` sync fallback (no running event loop) did not discard the chat from `_dirty_cursor_chats` after a successful write, leaving the chat marked dirty forever. Mirror the async branch. 3. (LOW) `test_at_cap_boundary_is_stale` actually exercised `cap + 1`, not the cap exactly. Renamed to `test_one_second_past_cap_is_stale` to match what's tested (a separate test covers within-cap behavior). 4. (LOW) `save_cursor()` docstring claimed failures were "logged and re-raised", but the function only re-raises — callers log. Updated the docstring to match actual behavior. Two new regression tests added in tests/test_mcp_server_chat_cursors.py (seen_ids_tail determinism + sync-fallback dirty-set cleanup), both written TDD: failed first, then fix applied. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #17 — the background Teams poll's per-chat cursor (
last_ts,seen_ids,bootstrapped) used to live in-process only. Every MCP restart re-bootstrapped from "newest message at boot" — surfacing days-old messages as fresh, or silently dropping messages that arrived during a server-down window.src/entrabot/tools/chat_cursors.py—load_cursor/save_cursor/is_stale/bound_seen_ids. Persists through the sameMemoryBackendprotocol (ADR-005 Phase 2) asinteraction_log.py/daily_summary.py. One key per chat atchat_cursors/<chat_id>.jsonso writes are independent._register_watched_chatrehydrates a fresh cursor (within 24h staleness cap) into_state["watched_chats"][chat_id]and skips_bootstrap_chat. Absent / stale / corrupt cursors fall through to the existing bootstrap path — first-time-registration semantics are unchanged._background_pollschedules a debounced async save (~1s) after every cycle that advanceslast_tsorseen_ids. Bursts in chatty chats coalesce to one backend write._bootstrap_chatpersists its watermark so a restart doesn't re-baseline._flush_chat_cursors()in thefinallyblock so dirty cursors land before exit.seen_ids_tailbounded to ~50 most recent IDs on serialize;last_tscarries everything older.Staleness cap: 24 hours on
last_ts. Configurable later if needed, but this is the policy the issue calls for and the failure mode it directly addresses.Cloud-mode is automatic:
BLOB_ENDPOINT+BLOB_CONTAINER→ cursors ride the same blob container as the rest of operational state. No persona-sati involvement (this is operational state, wrong bucket).Test plan
pytest -v --tb=short— 1278 passing, 1 skipped (was 1243; +35).ruff check .— clean.LocalBackend(corrupt JSON tolerated, per-chat keys independent,seen_ids_tailbounded on write)._register_watched_chatrehydrates fresh cursors and skips bootstrap; stale/absent/corrupt fall through._flush_chat_cursorswrites every dirty chat; a single chat's write failure does not block the rest.~/.entrabot/data/chat_cursors/<chat_id>.jsonmatches the prior cursor.Files changed
src/entrabot/tools/chat_cursors.py(new)src/entrabot/mcp_server.py(rehydrate + schedule + flush wiring)tests/tools/test_chat_cursors.py(new, 25 tests)tests/test_mcp_server_chat_cursors.py(new, 10 tests)docs/engineering-status.md(Recently Shipped + test count)Refs #17.
🤖 Generated with Claude Code