diff --git a/routes/email_routes.py b/routes/email_routes.py index 76a744ce1..81136e36e 100644 --- a/routes/email_routes.py +++ b/routes/email_routes.py @@ -2861,13 +2861,22 @@ def setup_email_routes(): return indexed_response return {"emails": [], "total": 0, "error": "Mail operation failed"} - def _read_email_sync(uid, folder, account_id, owner, mark_seen=True, full=False): + def _read_email_sync(uid, folder, account_id, owner, mark_seen=False, full=False): """Sync IMAP read — wrapped in to_thread by the async handler. The normal reader path fetches the headers plus a bounded body prefix. That avoids downloading multi-megabyte attachments just to open a message. Full-message fetch remains available for flows that need attachment metadata immediately, such as forwarding. + + `mark_seen` defaults to False because it mutates provider state: it + selects the mailbox read-write and issues a STORE. Only a foreground + open should ask for it, and it has to ask explicitly. + + A failed \\Seen transition is reported as `mark_seen_failed` on an + otherwise normal response, never as an error. The body has already been + fetched at that point, so refusing to return it would turn a cosmetic + flag failure into an unreadable message. """ import time as _t _t0 = _t.monotonic() @@ -2875,9 +2884,28 @@ def setup_email_routes(): preview_bytes = 384 * 1024 _t_select = 0.0 _t_fetch = 0.0 + mark_seen_failed = False try: with _imap(account_id, owner=owner) as conn: - conn.select(_q(folder), readonly=True) + # A foreground open owns both the body fetch and the \Seen + # transition. Keep them on one read-write IMAP selection so the + # route never schedules a second connection that can race the + # response. Prefetch/read-only callers retain BODY.PEEK and a + # read-only mailbox selection. + try: + conn.select(_q(folder), readonly=not mark_seen) + except Exception as select_exc: + if not mark_seen: + raise + # Read-only mailboxes (shared archives, some provider + # folders) reject a read-write SELECT. Serve the message + # read-only and report the flag failure. + logger.warning( + f"read-write SELECT rejected for {folder!r}; " + f"serving read-only without \\Seen: {select_exc}" + ) + conn.select(_q(folder), readonly=True) + mark_seen_failed = True _t_select = _t.monotonic() - _t0 fetch_query = "(BODY.PEEK[])" if full else f"(BODY.PEEK[HEADER] BODY.PEEK[TEXT]<0.{preview_bytes}>)" status, msg_data = _imap_uid_fetch(conn, uid, fetch_query) @@ -2903,22 +2931,44 @@ def setup_email_routes(): header_part = msg_data[0][1] or b"" raw = header_part + b"\r\n" + text_part - msg = email_mod.message_from_bytes(raw) + # Parse the fetched payload before mutating provider state. If + # the message is malformed enough that the reader cannot build + # a response, the caller gets an error while the message stays + # unread instead of receiving a false optimistic rollback. + msg = email_mod.message_from_bytes(raw) - subject = _decode_header(msg.get("Subject", "(no subject)")) - sender = _decode_header(msg.get("From", "unknown")) - to = _decode_header(msg.get("To", "")) - cc = _decode_header(msg.get("Cc", "")) - date_str = msg.get("Date", "") - message_id = msg.get("Message-ID", "") - in_reply_to = msg.get("In-Reply-To", "") - references = msg.get("References", "") - body = _extract_text(msg) - body_html = _extract_html(msg) + subject = _decode_header(msg.get("Subject", "(no subject)")) + sender = _decode_header(msg.get("From", "unknown")) + to = _decode_header(msg.get("To", "")) + cc = _decode_header(msg.get("Cc", "")) + date_str = msg.get("Date", "") + message_id = msg.get("Message-ID", "") + in_reply_to = msg.get("In-Reply-To", "") + references = msg.get("References", "") + body = _extract_text(msg) + body_html = _extract_html(msg) + + sender_name, sender_addr = email.utils.parseaddr(sender) + parsed_date = email.utils.parsedate_to_datetime(date_str) if date_str else None + attachments = _list_attachments_from_msg(msg) if full else (_email_attachment_meta_cache_get(owner, account_id, folder, uid) or []) + + if mark_seen and not mark_seen_failed: + seen_status, _ = conn.uid("STORE", _uid_bytes(uid), "+FLAGS", "(\\Seen)") + if seen_status != "OK": + # Report, don't raise. The parsed body below is still a + # valid response; only the flag claim is untrue. + logger.warning( + f"IMAP STORE \\Seen failed for UID {uid} in {folder!r}: {seen_status}" + ) + mark_seen_failed = True + + # Only record the local flag transition when the provider actually + # accepted it, so the index and list cache cannot drift ahead of + # the mailbox. + if mark_seen and not mark_seen_failed: + _email_index_update_flags(owner, account_id, folder, uid, "\\Seen", True) + _update_list_cache_seen(account_id, folder, uid, True) - sender_name, sender_addr = email.utils.parseaddr(sender) - parsed_date = email.utils.parsedate_to_datetime(date_str) if date_str else None - attachments = _list_attachments_from_msg(msg) if full else (_email_attachment_meta_cache_get(owner, account_id, folder, uid) or []) related_attachments = [] if full and not _has_visible_attachments(msg): related_attachments = _related_thread_attachments_sync( @@ -3039,20 +3089,29 @@ def setup_email_routes(): "boundaries": cached_boundaries, "thread_turns": cached_turns, "sender_signature": cached_sender_sig, + # Per-request, not part of the message: the route strips this + # before caching so a one-off flag failure is never replayed to + # later readers. + "mark_seen_failed": mark_seen_failed, } except Exception as e: logger.error(f"Failed to read email {uid}: {e}") return {"error": "Mail operation failed"} def _mark_email_seen_sync(uid, folder, account_id, owner): + """Synchronously mark a cached email seen and report success.""" try: with _imap(account_id, owner=owner) as conn: - conn.select(_q(folder)) - conn.uid("STORE", _uid_bytes(uid), "+FLAGS", "\\Seen") + conn.select(_q(folder), readonly=False) + status, _ = conn.uid("STORE", _uid_bytes(uid), "+FLAGS", "(\\Seen)") + if status != "OK": + return False _email_index_update_flags(owner, account_id, folder, uid, "\\Seen", True) _update_list_cache_seen(account_id, folder, uid, True) + return True except Exception as e: - logger.debug(f"mark-seen after cached read failed uid={uid}: {e}") + logger.warning(f"mark-seen after cached read failed uid={uid}: {e}") + return False @router.get("/read/{uid}") async def read_email_by_uid( @@ -3078,32 +3137,32 @@ def setup_email_routes(): if cached.get("attachment_version") != EMAIL_READ_ATTACHMENT_VERSION: cached = None if cached is not None: - if mark_seen: - try: - _asyncio.create_task(_asyncio.to_thread(_mark_email_seen_sync, uid, folder, account_id, owner)) - except RuntimeError: - pass + # A cache hit already holds a complete, valid message. Await the + # STORE so the response reports the real flag state, but never let + # a failed STORE withhold a body we are holding in memory. + if mark_seen and not await _asyncio.to_thread( + _mark_email_seen_sync, uid, folder, account_id, owner + ): + return {**cached, "mark_seen_failed": True} return cached if not full: persisted = _email_preview_cache_get(owner, account_id, folder, uid) if persisted and persisted.get("attachment_version") == EMAIL_READ_ATTACHMENT_VERSION: _read_cache_put(ck, persisted) - if mark_seen: - try: - _asyncio.create_task(_asyncio.to_thread(_mark_email_seen_sync, uid, folder, account_id, owner)) - except RuntimeError: - pass + if mark_seen and not await _asyncio.to_thread( + _mark_email_seen_sync, uid, folder, account_id, owner + ): + return {**persisted, "mark_seen_failed": True} return persisted result = await _asyncio.to_thread(_read_email_sync, uid, folder, account_id, owner, mark_seen, full) if result and not result.get("error"): - _read_cache_put(ck, result) + # `mark_seen_failed` describes this request, not the message, so it + # must not enter either cache — a later reader would otherwise be + # told a STORE failed that it never issued. + cacheable = {k: v for k, v in result.items() if k != "mark_seen_failed"} + _read_cache_put(ck, cacheable) if not full: - _email_preview_cache_put(owner, account_id, folder, uid, result) - if mark_seen: - try: - _asyncio.create_task(_asyncio.to_thread(_mark_email_seen_sync, uid, folder, account_id, owner)) - except RuntimeError: - pass + _email_preview_cache_put(owner, account_id, folder, uid, cacheable) return result def _schedule_recent_email_warm(emails: list, folder: str, account_id: str | None, owner: str): diff --git a/static/js/emailInbox.js b/static/js/emailInbox.js index 605a5ff61..cfee9ff90 100644 --- a/static/js/emailInbox.js +++ b/static/js/emailInbox.js @@ -149,6 +149,7 @@ let _loading = false; let _expanded = false; let _docModule = null; let _listSpinner = null; +let _openEmailRequestSeq = 0; let _senderFilter = null; // email address (lowercased) to filter by, or null let _senderFilterLabel = null; // display label for the active filter chip let _showEmailTags = localStorage.getItem('odysseus.email.showTags') !== '0'; @@ -187,7 +188,7 @@ export function init(documentModule) { } catch (_) {} if (opts.compose) { _composeNew(); return; } if (opts.email) { - await _openEmail(opts.email, null, opts.emailData, opts.mode || 'reply', opts.noteHint || ''); + await _openEmail(opts.email, null, opts.emailData, opts.mode || 'reply', opts.noteHint || '', '', opts.mailboxContext || null); } }, }); @@ -751,7 +752,21 @@ function _createEmailItem(em) { return item; } -async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', noteHint = '', prefilledBody = '') { +async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', noteHint = '', prefilledBody = '', mailboxContext = null) { + const openRequestSeq = ++_openEmailRequestSeq; + const folderAtStart = mailboxContext?.messageFolder || _currentFolder; + const accountAtStart = mailboxContext?.accountId ?? (window.__odysseusActiveEmailAccount || ''); + const accountQueryAtStart = accountAtStart ? `&account_id=${encodeURIComponent(accountAtStart)}` : ''; + const mailboxContextIsCurrent = typeof mailboxContext?.isCurrent === 'function' + ? mailboxContext.isCurrent + : () => ( + folderAtStart === _currentFolder && + accountAtStart === (window.__odysseusActiveEmailAccount || '') + ); + const isCurrentOpen = () => ( + openRequestSeq === _openEmailRequestSeq && + mailboxContextIsCurrent() + ); const aiReplyMode = mode === 'ai-reply-fast' ? 'fast' : ''; const wantsAiReply = mode === 'ai-reply' || !!aiReplyMode; // Body pre-fill from the agent's open_email_reply tool call takes the @@ -780,9 +795,10 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note let data = preloadedData; if (!data) { const fullQS = mode === 'forward' ? '&full=1' : ''; - const res = await fetch(`${API_BASE}/api/email/read/${em.uid}?folder=${encodeURIComponent(_currentFolder)}${_acct()}${fullQS}`); + const res = await fetch(`${API_BASE}/api/email/read/${em.uid}?folder=${encodeURIComponent(folderAtStart)}${accountQueryAtStart}&mark_seen=true${fullQS}`); data = await res.json(); } + if (!isCurrentOpen()) return; if (data.error) { console.error('Failed to read email:', data.error); return; @@ -808,7 +824,7 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note message_id: _fallback(data.message_id, em.message_id), }; if (wantsAiReply) { - const activeReplyAccount = data.account_id || em.account_id || window.__odysseusActiveEmailAccount || ''; + const activeReplyAccount = data.account_id || em.account_id || accountAtStart; if (data.cached_ai_reply && !noteHint && !activeReplyAccount) { aiSuggestedBody = _cleanAiReplyText(data.cached_ai_reply); } else { @@ -834,7 +850,7 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note session_id: currentSessionId, message_id: data.message_id || '', uid: String(em.uid || ''), - folder: _currentFolder, + folder: folderAtStart, account_id: activeReplyAccount, fast: true, user_hint: (noteHint || '').trim() || undefined, @@ -842,6 +858,7 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note }); const result = await res.json(); if (draftToastTimer) clearTimeout(draftToastTimer); + if (!isCurrentOpen()) return; if (result.success && result.reply) { aiSuggestedBody = _cleanAiReplyText(result.reply); } else { @@ -855,6 +872,7 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note } } catch (e) { if (draftToastTimer) clearTimeout(draftToastTimer); + if (!isCurrentOpen()) return; console.error('AI reply generation failed:', e); import('./ui.js').then(m => m.showError && m.showError('AI reply failed: ' + (e.message || e))).catch(() => {}); return; @@ -862,8 +880,12 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note } } - em.is_read = true; - if (itemEl) itemEl.classList.remove('email-unread'); + if (!isCurrentOpen()) return; + // Only claim the message is read when the provider accepted the \Seen + // transition. A failed STORE still opens the message; it just stays unread. + const markedSeen = !data.mark_seen_failed; + em.is_read = markedSeen; + if (itemEl) itemEl.classList.toggle('email-unread', !markedSeen); // Addresses to exclude from Reply All. Prefer the full set of configured // accounts (so a multi-account user's other mailboxes are excluded too), @@ -911,7 +933,7 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note if (mode !== 'forward' && data.message_id) content += `\nIn-Reply-To: ${data.message_id}`; if (mode !== 'forward' && data.message_id) content += `\nReferences: ${data.references ? data.references + ' ' + data.message_id : data.message_id}`; content += `\nX-Source-UID: ${em.uid}`; - content += `\nX-Source-Folder: ${_currentFolder}`; + content += `\nX-Source-Folder: ${folderAtStart}`; if (data.attachments && data.attachments.length > 0) { const attStr = data.attachments.map(a => `${a.index}:${a.filename}:${a.size}`).join('|'); content += `\nX-Attachments: ${attStr}`; @@ -980,21 +1002,27 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note // and block Send on long threads. const reuseExisting = mode !== 'forward' && !!aiSuggestedBody; const existingDocId = (reuseExisting && _docModule.findEmailDocId) - ? _docModule.findEmailDocId(em.uid, _currentFolder) + ? _docModule.findEmailDocId(em.uid, folderAtStart) : null; if (existingDocId) { if (!_docModule.isPanelOpen()) _docModule.openPanel(); await new Promise(r => requestAnimationFrame(() => requestAnimationFrame(r))); + if (!isCurrentOpen()) return; await _docModule.loadDocument(existingDocId); + if (!isCurrentOpen()) return; if (typeof _docModule.ensureEmailDraftEnvelope === 'function') { await _docModule.ensureEmailDraftEnvelope(existingDocId, content); + if (!isCurrentOpen()) return; } if (aiSuggestedBody && typeof _docModule.replaceEmailReplyBody === 'function') { await _docModule.replaceEmailReplyBody(existingDocId, aiSuggestedBody, { force: false }); + if (!isCurrentOpen()) return; } _bringEmailReplyDraftToFrontOnMobile(); } else { + if (!isCurrentOpen()) return; let activeSid = await _createEmailChat(data, { forceNew: true }); + if (!isCurrentOpen()) return; if (!activeSid) { console.error('reply: could not obtain a session_id'); import('./ui.js').then(m => m.showError && m.showError('Could not start a reply chat.')).catch(() => {}); @@ -1012,13 +1040,20 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note }), }); let docRes = await createReplyDoc(activeSid); + if (!isCurrentOpen()) return; if (docRes.status === 404) { console.warn('[reply-debug] draft session rejected; retrying in a fresh email chat', activeSid); + if (!isCurrentOpen()) return; activeSid = await _createEmailChat(data, { forceNew: true }); - if (activeSid) docRes = await createReplyDoc(activeSid); + if (!isCurrentOpen()) return; + if (activeSid) { + docRes = await createReplyDoc(activeSid); + if (!isCurrentOpen()) return; + } } if (!docRes.ok) { const errText = await docRes.text(); + if (!isCurrentOpen()) return; console.error('[reply-debug] POST /api/document failed', docRes.status, errText); // uiModule isn't statically imported here — use the dynamic // import pattern the rest of this file uses. (Previously this @@ -1028,10 +1063,12 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note return; } const doc = await docRes.json(); + if (!isCurrentOpen()) return; if (doc.id) { const wasOpen = _docModule.isPanelOpen(); if (!wasOpen) _docModule.openPanel(); await new Promise(r => requestAnimationFrame(() => requestAnimationFrame(r))); + if (!isCurrentOpen()) return; // Use the doc dict from the POST directly — avoids a 404 race // when the GET fires before the new row is visible to the read // connection (or when caching is interfering). loadDocument's @@ -1040,12 +1077,14 @@ async function _openEmail(em, itemEl, preloadedData = null, mode = 'reply', note _docModule.injectFreshDoc(doc); } else { await _docModule.loadDocument(doc.id); + if (!isCurrentOpen()) return; } _bringEmailReplyDraftToFrontOnMobile(); } } } } catch (e) { + if (!isCurrentOpen()) return; console.error('Failed to open email:', e); // Surface the failure so a silent throw in the reply flow doesn't // look like "nothing happened". Dynamic import — uiModule isn't a diff --git a/static/js/emailLibrary.js b/static/js/emailLibrary.js index 32b906ddc..9425a0181 100644 --- a/static/js/emailLibrary.js +++ b/static/js/emailLibrary.js @@ -30,6 +30,10 @@ import { bindMenuDismiss, dismissOrRemove } from './escMenuStack.js'; const API_BASE = window.location.origin; let _emailUnreadChipClickWired = false; let _libLoadSeq = 0; +let _emailMailboxGeneration = 0; +let _emailCardOpenSeq = 0; +let _emailReadMutationSeq = 0; +const _emailReadMutations = new Map(); let _libFolderSeq = 0; let _libSearchSeq = 0; let _libSearchHadResults = false; @@ -837,14 +841,41 @@ document.addEventListener('keydown', (e) => { e.stopImmediatePropagation?.(); }, true); -function _syncEmailReadState(uid, isRead = true) { +function _emailReadContextKey(context) { + return [context.accountId, context.folder, context.uid].map(value => String(value || '')).join('\u0000'); +} + +function _emailReadContextIsCurrent(context) { + if (!context) return true; + return ( + String(state._libAccountId || '') === context.accountId && + String(state._libFolder || 'INBOX') === context.libraryFolder && + _emailMailboxGeneration === context.mailboxGeneration + ); +} + +function _emailMatchesReadContext(email, context) { + if (String(email?.uid || '') !== context.uid) return false; + const accountId = String(email?.account_id || context.accountId); + const folder = String(email?.folder || context.folder); + return accountId === context.accountId && folder === context.folder; +} + +function _syncEmailReadState(uid, isRead = true, context = null) { if (uid == null) return; const uidStr = String(uid); const read = !!isRead; - const match = (state._libEmails || []).find(x => String(x.uid) === uidStr); + if (context && (!_emailReadContextIsCurrent(context) || uidStr !== context.uid)) return; + const match = (state._libEmails || []).find(x => ( + context ? _emailMatchesReadContext(x, context) : String(x.uid) === uidStr + )); if (match) match.is_read = read; document.querySelectorAll('.doclib-card[data-uid="' + CSS.escape(uidStr) + '"]').forEach(card => { + if (context && ( + String(card.dataset.emailAccount || '') !== context.accountId || + String(card.dataset.emailFolder || '') !== context.folder + )) return; card.classList.toggle('email-card-unread', !read); const titleRow = card.querySelector('.email-card-titlerow'); if (read) { @@ -1908,6 +1939,7 @@ function _resetEmailListForFreshLoad({ useCache = true } = {}) { _exitEmailReaderModeForList(); _resetBulkSelectionForContextChange(); state._libOffset = 0; + _emailMailboxGeneration += 1; _libLoadSeq += 1; const ck = _libCacheKey(); const cached = useCache ? _libCacheGet(ck) : null; @@ -2286,7 +2318,25 @@ function _publishActiveAccount() { export function initEmailLibrary(config) { state._docModule = config.documentModule; - state._onEmailClick = config.onEmailClick; + const onEmailClick = config.onEmailClick; + state._onEmailClick = typeof onEmailClick === 'function' ? (options = {}) => { + const accountId = String(state._libAccountId || ''); + const libraryFolder = String(state._libFolder || 'INBOX'); + const messageFolder = String(options.email?.folder || libraryFolder); + const mailboxGeneration = _emailMailboxGeneration; + const mailboxContext = Object.freeze({ + accountId, + libraryFolder, + messageFolder, + mailboxGeneration, + isCurrent: () => ( + String(state._libAccountId || '') === accountId && + String(state._libFolder || 'INBOX') === libraryFolder && + _emailMailboxGeneration === mailboxGeneration + ), + }); + return onEmailClick({ ...options, mailboxContext }); + } : null; } export function isOpen() { return state._libOpen; } @@ -2303,6 +2353,7 @@ export function openEmailLibrary(opts = {}) { document.removeEventListener('keydown', state._libEscHandler, true); state._libEscHandler = null; } + _emailMailboxGeneration += 1; state._libOpen = true; // On mobile the sidebar overlays content — close it so the email view isn't // opened behind it (same pattern as session-switch/delete). @@ -4836,6 +4887,8 @@ function _createCard(em) { else if (!em.is_read) cls += ' email-card-unread'; card.className = cls; card.dataset.uid = String(em.uid); + card.dataset.emailAccount = String(em.account_id || state._libAccountId || ''); + card.dataset.emailFolder = String(em.folder || state._libFolder || 'INBOX'); if (state._selectMode && state._selectedUids.has(em.uid)) card.classList.add('selected'); // Checkbox in select mode @@ -5162,6 +5215,25 @@ async function _toggleCardPreview(card, em) { // currently-selected folder for normal inbox cards. const folderAtStart = (em && em.folder) || libraryFolderAtStart; const uidAtStart = String(em?.uid || card?.dataset?.uid || ''); + const wasReadAtStart = !!em?.is_read; + const openGeneration = ++_emailCardOpenSeq; + const readContext = Object.freeze({ + accountId: String(accountAtStart), + libraryFolder: String(libraryFolderAtStart), + folder: String(folderAtStart), + uid: uidAtStart, + mailboxGeneration: _emailMailboxGeneration, + }); + const readContextKey = _emailReadContextKey(readContext); + const isCurrentOpen = () => ( + openGeneration === _emailCardOpenSeq && + _emailReadContextIsCurrent(readContext) && + accountAtStart === (state._libAccountId || '') && + libraryFolderAtStart === (state._libFolder || 'INBOX') && + uidAtStart === String(card?.dataset?.uid || '') && + card.isConnected && + card.classList.contains('email-card-expanded') + ); const grid = card.closest('.doclib-grid'); const gridRect = grid?.getBoundingClientRect?.(); const modal = document.getElementById('email-lib-modal'); @@ -5186,6 +5258,30 @@ async function _toggleCardPreview(card, em) { return; } + // Every authoritative open supersedes any older optimistic mutation for the + // same immutable mailbox identity. Carry the original unread state forward + // so a close/reopen followed by failure still rolls back exactly once, while + // a late failure from the superseded request cannot undo a newer success. + const previousMutation = _emailReadMutations.get(readContextKey); + const readMutation = { + generation: ++_emailReadMutationSeq, + rollbackUnread: !wasReadAtStart || !!previousMutation?.rollbackUnread, + }; + _emailReadMutations.set(readContextKey, readMutation); + const restoreUnreadState = () => { + if (_emailReadMutations.get(readContextKey)?.generation !== readMutation.generation) return; + _emailReadMutations.delete(readContextKey); + if (readMutation.rollbackUnread) _syncEmailReadState(uidAtStart, false, readContext); + }; + const commitReadState = () => { + // A successful STORE/mark_seen is authoritative for this immutable + // mailbox identity even when a newer open is still pending. Retire that + // newer rollback token too, otherwise its later failure could restore an + // unread state that no longer exists at the provider. + _emailReadMutations.delete(readContextKey); + _syncEmailReadState(uidAtStart, true, readContext); + }; + // Collapse any other expanded card if (grid) { grid.querySelectorAll('.email-card-expanded').forEach(c => { @@ -5207,10 +5303,10 @@ async function _toggleCardPreview(card, em) { requestAnimationFrame(() => { try { card.scrollIntoView({ behavior: 'smooth', block: 'start' }); } catch (_) {} }); - if (!em.is_read) { - _syncEmailReadState(em.uid, true); - fetch(`${API_BASE}/api/email/mark-read/${em.uid}?folder=${encodeURIComponent(folderAtStart)}${_acct()}`, { method: 'POST' }) - .catch(err => console.error('Failed to mark email read:', err)); + if (!wasReadAtStart) { + // Keep the current optimistic visual update, but let the read request below + // own the provider-side \Seen transition. A failure restores unread state. + _syncEmailReadState(uidAtStart, true, readContext); } // Class hook on the modal so the header-hide / padding rules work on // browsers without :has() support (Firefox mobile) — the :has() versions @@ -5239,25 +5335,28 @@ async function _toggleCardPreview(card, em) { } catch (_) {} }; + let authoritativeReadSucceeded = false; try { - const res = await fetch(`${API_BASE}/api/email/read/${em.uid}?folder=${encodeURIComponent(folderAtStart)}${_acct()}`); + const accountQueryAtStart = accountAtStart ? `&account_id=${encodeURIComponent(accountAtStart)}` : ''; + const res = await fetch(`${API_BASE}/api/email/read/${encodeURIComponent(uidAtStart)}?folder=${encodeURIComponent(folderAtStart)}${accountQueryAtStart}&mark_seen=true`); if (!res.ok) throw new Error(`HTTP ${res.status}`); const data = await res.json(); - if ( - accountAtStart !== (state._libAccountId || '') || - libraryFolderAtStart !== (state._libFolder || 'INBOX') || - uidAtStart !== String(card?.dataset?.uid || '') || - !card.isConnected || - !card.classList.contains('email-card-expanded') - ) { - return; - } if (data.error) { - showFailedReader(`Failed to load email: ${data.error}`); + restoreUnreadState(); + if (isCurrentOpen()) showFailedReader(`Failed to load email: ${data.error}`); return; } - // Mark as read locally - _syncEmailReadState(em.uid, true); + if (data.mark_seen_failed) { + // The body is authoritative even when the provider refused the \Seen + // transition. Render the message and roll the unread marker back so the + // list keeps telling the truth, rather than refusing to open a message + // we successfully read. + restoreUnreadState(); + } else { + authoritativeReadSucceeded = true; + commitReadState(); + } + if (!isCurrentOpen()) return; _stampReaderContext(reader, { ...em, ...data }, state._libFolder, state._libAccountId); // Build the attachments wrap using the shared helper so the signature- @@ -5439,7 +5538,10 @@ async function _toggleCardPreview(card, em) { // Always stop bubbling so the card's click doesn't fire while reading. reader.addEventListener('click', (ev) => { ev.stopPropagation(); }); } catch (e) { - showFailedReader(e?.message ? `Failed to load email: ${e.message}` : 'Failed to load email'); + if (!authoritativeReadSucceeded) restoreUnreadState(); + if (isCurrentOpen()) { + showFailedReader(e?.message ? `Failed to load email: ${e.message}` : 'Failed to load email'); + } } } diff --git a/tests/test_email_open_dedup_js.py b/tests/test_email_open_dedup_js.py new file mode 100644 index 000000000..cc6c431f7 --- /dev/null +++ b/tests/test_email_open_dedup_js.py @@ -0,0 +1,231 @@ +"""Focused browser-side regression coverage for authoritative email opens.""" + +import json +import shutil +import subprocess +from pathlib import Path + +import pytest + + +_REPO = Path(__file__).resolve().parent.parent +_INBOX_JS = _REPO / "static" / "js" / "emailInbox.js" +_LIBRARY_JS = _REPO / "static" / "js" / "emailLibrary.js" +_HAS_NODE = shutil.which("node") is not None + + +def _extract_between(source: str, signature: str, next_marker: str) -> str: + start = source.index(signature) + end = source.index(next_marker, start) + return source[start:end].rstrip() + + +def test_library_unread_preview_has_one_authoritative_request_and_rollback(): + source = _LIBRARY_JS.read_text(encoding="utf-8") + function = _extract_between(source, "async function _toggleCardPreview", "\n/**\n * Wrap a probable signature block") + + assert function.count("/api/email/read/") == 1 + assert "/api/email/mark-read/" not in function + assert "&mark_seen=true" in function + assert "_syncEmailReadState(uidAtStart, true, readContext)" in function + assert "_syncEmailReadState(uidAtStart, false, readContext)" in function + assert "openGeneration === _emailCardOpenSeq" in function + assert "_emailReadMutations.get(readContextKey)?.generation !== readMutation.generation" in function + assert "authoritativeReadSucceeded = true;" in function + assert "if (!authoritativeReadSucceeded) restoreUnreadState();" in function + assert "if (!isCurrentOpen()) return" in function + + +@pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") +def test_library_authoritative_success_defeats_newer_rollback_in_either_order(): + source = _LIBRARY_JS.read_text(encoding="utf-8") + function = _extract_between(source, "async function _toggleCardPreview", "\n/**\n * Wrap a probable signature block") + settlements = _extract_between( + function, + " const restoreUnreadState = () => {", + "\n\n // Collapse any other expanded card", + ) + + harness = f""" +const _emailReadMutations = new Map(); +const readContextKey = 'same-mailbox-message'; +const uidAtStart = '1'; +const readContext = {{ accountId: 'acct-a', folder: 'INBOX', uid: '1' }}; +const readUpdates = []; +function _syncEmailReadState(uid, isRead, context) {{ + readUpdates.push({{ uid, isRead, context }}); +}} +function createSettlers(readMutation) {{ +{settlements} + return {{ restoreUnreadState, commitReadState }}; +}} +function runRace(successFirst) {{ + _emailReadMutations.clear(); + readUpdates.length = 0; + const mutationA = {{ generation: 1, rollbackUnread: true }}; + _emailReadMutations.set(readContextKey, mutationA); + const settlersA = createSettlers(mutationA); + const mutationB = {{ generation: 2, rollbackUnread: true }}; + _emailReadMutations.set(readContextKey, mutationB); + const settlersB = createSettlers(mutationB); + if (successFirst) {{ + settlersA.commitReadState(); + settlersB.restoreUnreadState(); + }} else {{ + settlersB.restoreUnreadState(); + settlersA.commitReadState(); + }} + return {{ + hasMutation: _emailReadMutations.has(readContextKey), + readUpdates: readUpdates.map(update => update.isRead), + }}; +}} +console.log(JSON.stringify({{ + successFirst: runRace(true), + failureFirst: runRace(false), +}})); +""" + proc = subprocess.run( + ["node", "--input-type=module"], + input=harness, + capture_output=True, + text=True, + cwd=str(_REPO), + timeout=30, + ) + assert proc.returncode == 0, f"node failed: {proc.stderr}\n---\n{harness}" + assert json.loads(proc.stdout.strip()) == { + "successFirst": {"hasMutation": False, "readUpdates": [True]}, + "failureFirst": {"hasMutation": False, "readUpdates": [False, True]}, + } + + +def test_library_reply_open_carries_immutable_mailbox_context(): + library_source = _LIBRARY_JS.read_text(encoding="utf-8") + inbox_source = _INBOX_JS.read_text(encoding="utf-8") + + assert "const mailboxGeneration = _emailMailboxGeneration;" in library_source + assert "messageFolder = String(options.email?.folder || libraryFolder)" in library_source + assert "return onEmailClick({ ...options, mailboxContext });" in library_source + assert "mailboxContext?.messageFolder || _currentFolder" in inbox_source + assert "mailboxContextIsCurrent()" in inbox_source + assert "if (!isCurrentOpen()) return;\n let activeSid = await _createEmailChat" in inbox_source + + +@pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") +def test_inbox_late_read_response_cannot_apply_after_newer_open(): + source = _INBOX_JS.read_text(encoding="utf-8") + function = _extract_between(source, "async function _openEmail", "\nfunction _showEmailMenu") + assert "let _openEmailRequestSeq = 0;" in source + + harness = f""" +const realLog = console.log; +console.error = () => {{}}; +const API_BASE = 'https://odysseus.invalid'; +const window = {{ __odysseusActiveEmailAccount: 'acct-a' }}; +let _currentFolder = 'INBOX'; +const _acct = () => '&account_id=acct-a'; +let _openEmailRequestSeq = 0; +let _docModule = null; +const spinnerModule = {{ createWhirlpool() {{ throw new Error('spinner should not run'); }} }}; +const sessionModule = null; +let firstResolve; +const calls = []; +async function fetch(url) {{ + calls.push(String(url)); + if (calls.length === 1) {{ + return await new Promise((resolve) => {{ + firstResolve = () => resolve({{ json: async () => ({{ uid: '1', subject: 'old' }}) }}); + }}); + }} + return {{ json: async () => ({{ error: 'newer open completed test' }}) }}; +}} +{function} +const oldEmail = {{ uid: '1', is_read: false }}; +const newerEmail = {{ uid: '2', is_read: false }}; +const first = _openEmail(oldEmail, null); +await Promise.resolve(); +const second = _openEmail(newerEmail, null); +await second; +firstResolve(); +await first; +realLog(JSON.stringify({{ calls, oldRead: oldEmail.is_read, newerRead: newerEmail.is_read }})); +""" + proc = subprocess.run( + ["node", "--input-type=module"], + input=harness, + capture_output=True, + text=True, + cwd=str(_REPO), + timeout=30, + ) + assert proc.returncode == 0, f"node failed: {proc.stderr}\n---\n{harness}" + result = json.loads(proc.stdout.strip()) + assert len(result["calls"]) == 2 + assert all("mark_seen=true" in url for url in result["calls"]) + assert result["oldRead"] is False + assert result["newerRead"] is False + + +@pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") +@pytest.mark.parametrize("context_change", ["account", "folder", "library"]) +def test_inbox_late_read_response_cannot_apply_after_mailbox_switch(context_change): + source = _INBOX_JS.read_text(encoding="utf-8") + function = _extract_between(source, "async function _openEmail", "\nfunction _showEmailMenu") + + changes = { + "account": "window.__odysseusActiveEmailAccount = 'acct-b';", + "folder": "_currentFolder = 'Archive';", + "library": "libraryCurrent = false;", + } + change = changes[context_change] + open_call = ( + "_openEmail(email, null, null, 'reply', '', '', mailboxContext)" + if context_change == "library" + else "_openEmail(email, null)" + ) + harness = f""" +const realLog = console.log; +console.error = () => {{}}; +const API_BASE = 'https://odysseus.invalid'; +const window = {{ __odysseusActiveEmailAccount: 'acct-a' }}; +let _currentFolder = 'INBOX'; +const _acct = () => '&account_id=acct-a'; +let _openEmailRequestSeq = 0; +let libraryCurrent = true; +const mailboxContext = {{ + accountId: 'acct-a', + messageFolder: 'Archive', + isCurrent: () => libraryCurrent, +}}; +let createCalls = 0; +let _docModule = {{}}; +async function _createEmailChat() {{ createCalls += 1; return 'stale-session'; }} +const spinnerModule = {{ createWhirlpool() {{ throw new Error('spinner should not run'); }} }}; +const sessionModule = null; +let resolveRead; +async function fetch() {{ + return await new Promise((resolve) => {{ + resolveRead = () => resolve({{ json: async () => ({{ uid: '1', subject: 'old' }}) }}); + }}); +}} +{function} +const email = {{ uid: '1', is_read: false }}; +const pending = {open_call}; +await Promise.resolve(); +{change} +resolveRead(); +await pending; +realLog(JSON.stringify({{ createCalls, isRead: email.is_read }})); +""" + proc = subprocess.run( + ["node", "--input-type=module"], + input=harness, + capture_output=True, + text=True, + cwd=str(_REPO), + timeout=30, + ) + assert proc.returncode == 0, f"node failed: {proc.stderr}\n---\n{harness}" + result = json.loads(proc.stdout.strip()) + assert result == {"createCalls": 0, "isRead": False} diff --git a/tests/test_email_read_mark_seen.py b/tests/test_email_read_mark_seen.py new file mode 100644 index 000000000..2d9faa273 --- /dev/null +++ b/tests/test_email_read_mark_seen.py @@ -0,0 +1,278 @@ +import asyncio +from contextlib import contextmanager + +import pytest + + +RAW_EMAIL = ( + b"From: Sender \r\n" + b"To: Alice \r\n" + b"Subject: Single authoritative open\r\n" + b"Message-ID: \r\n" + b"Date: Tue, 04 Aug 2026 12:00:00 +0000\r\n" + b"Content-Type: text/plain; charset=utf-8\r\n" + b"\r\n" + b"Body" +) + + +def _route_endpoint(router, path: str, method: str): + method = method.upper() + for route in router.routes: + if route.path == path and method in getattr(route, "methods", set()): + return route.endpoint + raise AssertionError(f"route not found: {method} {path}") + + +class FakeImap: + def __init__(self, store_status="OK", readonly_mailbox=False): + self.store_status = store_status + # Shared archives and some provider folders reject a read-write SELECT. + self.readonly_mailbox = readonly_mailbox + self.selects = [] + self.commands = [] + + def select(self, mailbox, readonly=False): + self.selects.append((mailbox, readonly)) + if self.readonly_mailbox and not readonly: + raise OSError("[READ-ONLY] Mailbox is read-only") + return "OK", [b"1"] + + def uid(self, command, uid, *args): + self.commands.append((command, uid, *args)) + if command == "FETCH": + header, body = RAW_EMAIL.split(b"\r\n\r\n", 1) + return "OK", [ + (b"1 (UID 42 BODY[HEADER])", header + b"\r\n\r\n"), + (b"1 (UID 42 BODY[TEXT]<0>)", body), + ] + if command == "STORE": + # RFC 3501 STORE takes a parenthesized flag-list. GreenMail rejects + # the formerly emitted bare ``\Seen`` atom with BAD, so keep the + # fake strict enough to catch that provider-compatibility failure. + if args != ("+FLAGS", "(\\Seen)"): + return "BAD", [b"Expected:'(' found:'\\'"] + return self.store_status, [] + raise AssertionError(f"unexpected IMAP command: {command}") + + +def _install_fakes(monkeypatch, tmp_path, *, store_status="OK", readonly_mailbox=False): + import routes.email_helpers as email_helpers + import routes.email_routes as email_routes + + db_path = tmp_path / "email.db" + monkeypatch.setattr(email_helpers, "SCHEDULED_DB", db_path) + monkeypatch.setattr(email_routes, "SCHEDULED_DB", db_path) + email_helpers._init_scheduled_db() + + connections = [] + indexed_updates = [] + + @contextmanager + def fake_imap(account_id=None, owner=""): + conn = FakeImap(store_status=store_status, readonly_mailbox=readonly_mailbox) + connections.append(conn) + yield conn + + monkeypatch.setattr(email_routes, "_start_poller", lambda: None) + monkeypatch.setattr(email_routes, "_imap", fake_imap) + monkeypatch.setattr(email_routes, "_email_preview_cache_get", lambda *_args, **_kwargs: None) + monkeypatch.setattr(email_routes, "_email_preview_cache_put", lambda *_args, **_kwargs: None) + monkeypatch.setattr(email_routes, "_email_attachment_meta_cache_get", lambda *_args, **_kwargs: None) + monkeypatch.setattr(email_routes, "_email_attachment_meta_cache_put", lambda *_args, **_kwargs: None) + monkeypatch.setattr( + email_routes, + "_email_index_update_flags", + lambda *args, **_kwargs: indexed_updates.append(args), + ) + return email_routes, connections, indexed_updates + + +@pytest.mark.asyncio +@pytest.mark.parametrize("mark_seen", [True, False]) +async def test_read_email_seen_contract_uses_one_imap_connection(monkeypatch, tmp_path, mark_seen): + email_routes, connections, indexed_updates = _install_fakes(monkeypatch, tmp_path) + router = email_routes.setup_email_routes() + read_email = _route_endpoint(router, "/api/email/read/{uid}", "GET") + + result = await read_email( + "42", + folder="INBOX", + account_id="acct-a", + mark_seen=mark_seen, + full=False, + owner="alice", + ) + + assert result["uid"] == "42" + assert len(connections) == 1 + conn = connections[0] + assert conn.selects == [(conn.selects[0][0], not mark_seen)] + assert [command[0] for command in conn.commands] == ( + ["FETCH", "STORE"] if mark_seen else ["FETCH"] + ) + assert "BODY.PEEK[HEADER]" in conn.commands[0][2] + if mark_seen: + assert conn.commands[1][2:] == ("+FLAGS", "(\\Seen)") + assert indexed_updates == [("alice", "acct-a", "INBOX", "42", "\\Seen", True)] + else: + assert indexed_updates == [] + + +@pytest.mark.asyncio +async def test_cached_read_awaits_one_seen_store_without_refetch(monkeypatch, tmp_path): + email_routes, connections, indexed_updates = _install_fakes(monkeypatch, tmp_path) + router = email_routes.setup_email_routes() + read_email = _route_endpoint(router, "/api/email/read/{uid}", "GET") + + first = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=False, full=False, owner="alice" + ) + monkeypatch.setattr( + asyncio, + "create_task", + lambda *_args, **_kwargs: (_ for _ in ()).throw( + AssertionError("cached mark-seen must be awaited, not scheduled") + ), + ) + second = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=True, full=False, owner="alice" + ) + + assert first["message_id"] == second["message_id"] + assert len(connections) == 2 + assert [command[0] for command in connections[0].commands] == ["FETCH"] + assert [command[0] for command in connections[1].commands] == ["STORE"] + assert connections[1].commands[0][2:] == ("+FLAGS", "(\\Seen)") + assert connections[1].selects[0][1] is False + assert indexed_updates == [("alice", "acct-a", "INBOX", "42", "\\Seen", True)] + + +@pytest.mark.asyncio +async def test_seen_store_failure_returns_the_body_and_reports_the_failure(monkeypatch, tmp_path): + """A failed STORE must not cost the reader the message. + + The body was fetched successfully before the flag update was attempted, so + the response stays a normal read and carries `mark_seen_failed` for the + client to roll its optimistic unread marker back. + """ + email_routes, connections, indexed_updates = _install_fakes( + monkeypatch, tmp_path, store_status="NO" + ) + router = email_routes.setup_email_routes() + read_email = _route_endpoint(router, "/api/email/read/{uid}", "GET") + + result = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=True, full=False, owner="alice" + ) + + assert "error" not in result + assert result["uid"] == "42" + assert result["mark_seen_failed"] is True + assert len(connections) == 1 + assert [command[0] for command in connections[0].commands] == ["FETCH", "STORE"] + # The local index must not claim a transition the provider rejected. + assert indexed_updates == [] + + +@pytest.mark.asyncio +async def test_read_only_mailbox_serves_the_message_without_marking_seen(monkeypatch, tmp_path): + """A mailbox that refuses a read-write SELECT is still readable. + + Opening the message is the user's actual goal; the \\Seen transition is a + side effect of it. A folder that cannot accept flag changes must therefore + fall back to a read-only selection rather than failing the open. + """ + email_routes, connections, indexed_updates = _install_fakes( + monkeypatch, tmp_path, readonly_mailbox=True + ) + router = email_routes.setup_email_routes() + read_email = _route_endpoint(router, "/api/email/read/{uid}", "GET") + + result = await read_email( + "42", folder="Archive", account_id="acct-a", mark_seen=True, full=False, owner="alice" + ) + + assert "error" not in result + assert result["uid"] == "42" + assert result["mark_seen_failed"] is True + # Read-write attempt first, then the read-only retry on the same connection. + assert [readonly for _mailbox, readonly in connections[0].selects] == [False, True] + # No STORE is attempted once the mailbox is known to be read-only. + assert [command[0] for command in connections[0].commands] == ["FETCH"] + assert indexed_updates == [] + + +@pytest.mark.asyncio +async def test_failed_seen_state_is_not_replayed_from_cache(monkeypatch, tmp_path): + """`mark_seen_failed` describes one request, not the stored message. + + A second read that does not ask to mark seen must come back clean, or every + later reader would inherit a STORE failure it never issued. + """ + email_routes, connections, _ = _install_fakes(monkeypatch, tmp_path, store_status="NO") + router = email_routes.setup_email_routes() + read_email = _route_endpoint(router, "/api/email/read/{uid}", "GET") + + failed = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=True, full=False, owner="alice" + ) + replayed = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=False, full=False, owner="alice" + ) + + assert failed["mark_seen_failed"] is True + assert replayed.get("mark_seen_failed", False) is False + assert replayed["uid"] == "42" + + +@pytest.mark.asyncio +async def test_unparseable_read_does_not_mark_seen(monkeypatch, tmp_path): + email_routes, connections, indexed_updates = _install_fakes(monkeypatch, tmp_path) + monkeypatch.setattr( + email_routes.email_mod, + "message_from_bytes", + lambda *_args, **_kwargs: (_ for _ in ()).throw(ValueError("malformed message")), + ) + router = email_routes.setup_email_routes() + read_email = _route_endpoint(router, "/api/email/read/{uid}", "GET") + + result = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=True, full=False, owner="alice" + ) + + assert result == {"error": "Mail operation failed"} + assert len(connections) == 1 + assert [command[0] for command in connections[0].commands] == ["FETCH"] + assert indexed_updates == [] + + +@pytest.mark.asyncio +async def test_cached_seen_store_failure_returns_the_cached_body(monkeypatch, tmp_path): + """A cache hit already holds a complete message; a failed STORE cannot take it away. + + This is the path where withholding the body would be least defensible — the + response is served from memory and needed no network at all. + """ + email_routes, connections, indexed_updates = _install_fakes( + monkeypatch, tmp_path, store_status="NO" + ) + router = email_routes.setup_email_routes() + read_email = _route_endpoint(router, "/api/email/read/{uid}", "GET") + + first = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=False, full=False, owner="alice" + ) + second = await read_email( + "42", folder="INBOX", account_id="acct-a", mark_seen=True, full=False, owner="alice" + ) + + assert first["uid"] == "42" + assert "error" not in second + assert second["uid"] == "42" + assert second["body"] == first["body"] + assert second["mark_seen_failed"] is True + assert len(connections) == 2 + assert [command[0] for command in connections[0].commands] == ["FETCH"] + assert [command[0] for command in connections[1].commands] == ["STORE"] + assert indexed_updates == []