fix(msteams): prefer freshest personal conversation for proactive DM sends#54702
Conversation
Greptile SummaryThis PR fixes a real production bug where MS Teams proactive DM sends could target a stale Bot Framework conversation ID when a user had multiple stored conversation references. The FS store's Key observations:
Confidence Score: 4/5
|
| user: { id: "group-user", aadObjectId: "shared-aad" }, | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 10)); |
There was a problem hiding this comment.
setTimeout timing creates a flaky ordering dependency
The test relies on a 10 ms setTimeout to guarantee that a:new-personal gets a strictly later lastSeenAt than a:old-personal. Under CI load or on systems with coarse timer resolution this gap can collapse to 0 ms, causing both entries to receive the same ISO string and making the sort non-deterministic.
A more reliable approach is to bypass the real clock altogether. For example, inject a synthetic clock or directly call upsert with an explicit lastSeenAt on the reference (if the store honours an incoming lastSeenAt). Alternatively, the upsert signature could accept an optional now override for testability.
At minimum, consider bumping the delay to something more generous (e.g. 50–100 ms) or adding an assertion that the two stored timestamps are actually different before checking the result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/conversation-store-fs.test.ts
Line: 148
Comment:
**`setTimeout` timing creates a flaky ordering dependency**
The test relies on a 10 ms `setTimeout` to guarantee that `a:new-personal` gets a strictly later `lastSeenAt` than `a:old-personal`. Under CI load or on systems with coarse timer resolution this gap can collapse to 0 ms, causing both entries to receive the same ISO string and making the sort non-deterministic.
A more reliable approach is to bypass the real clock altogether. For example, inject a synthetic clock or directly call `upsert` with an explicit `lastSeenAt` on the reference (if the store honours an incoming `lastSeenAt`). Alternatively, the `upsert` signature could accept an optional `now` override for testability.
At minimum, consider bumping the delay to something more generous (e.g. 50–100 ms) or adding an assertion that the two stored timestamps are actually different before checking the result.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good catch. I pushed a follow-up that does three things:
- mirrors the same freshest-personal-selection logic into
conversation-store-memory.tsso both store implementations behave the same - removes the redundant
& { lastSeenAt?: string }typing in the FS store now thatlastSeenAtlives onStoredConversationReference - removes the timer-based ordering from the test by making the regression coverage deterministic instead of relying on
setTimeout
Focused test still passes after the follow-up:
pnpm test -- extensions/msteams/src/conversation-store-fs.test.ts
Thanks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e6f25e6d8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| (parseTimestamp(b.reference.lastSeenAt) ?? 0) - | ||
| (parseTimestamp(a.reference.lastSeenAt) ?? 0) |
There was a problem hiding this comment.
Avoid ranking by mutable
lastSeenAt writes
findByUserId now orders matches by lastSeenAt, but that timestamp is updated on every upsert, including proactive send-side cache updates (send-context.ts calls store.upsert(...) when caching graphChatId). As a result, an older/stale personal conversation can be made “freshest” by a non-inbound write and keep winning this sort, so user:<aad> sends may continue targeting a stale Bot Framework conversation ID and still fail with 403 despite a newer valid DM reference existing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. You are right that send-side cache writes can currently interfere with the freshness ranking if they touch the same record. I started patching this by preserving/carrying forward the stored lastSeenAt on cache-only updates instead of letting those writes redefine freshness, but I want to tighten the regression coverage before I mark this thread resolved.
So for now: acknowledged, and I am treating this as a real follow-up to address on the PR rather than waving it away. Thanks.
|
Was Opus here? This is a very localized fix, I get sth better in. 
|
c00d20f to
4c6f312
Compare
|
Landed via temp rebase onto main.
Thanks @gumclaw! |
Summary
user:<aadObjectId>could resolve to a stale personal DM conversation reference when the same user had multiple stored references.findByUserIdnow considers all matching references, preferspersonalconversations, and then selects the freshestlastSeenAt; added a regression test covering multiple references for one user.lastSeenAton the stored reference.Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause / Regression History (if applicable)
git blame, prior PR, issue, or refactor if known): Unknown.Regression Test Plan (if applicable)
extensions/msteams/src/conversation-store-fs.test.tsUser-visible / Behavior Changes
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
Steps
findByUserIdfor that user.Expected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
pnpm buildpasses.pnpm checkandpnpm testgreen, becauseorigin/maincurrently has unrelated TUI type errors insrc/tui/components/*forgetEditorKeybindings.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoFailure Recovery (if this breaks)
findByUserIdselection behavior.extensions/msteams/src/conversation-store-fs.ts,extensions/msteams/src/conversation-store.tsRisks and Mitigations
personaland newestlastSeenAtcould change resolution behavior for edge cases where a non-personal reference was intentionally preferred.user:<id>lookup, which is used for person-targeted proactive sends; the new regression test locks the expected DM-preference behavior.