← Back to PRs

#12414: fix(slack): do not cache API failures in thread_ts resolver

by Yida-Dev open 2026-02-09 07:05 View on GitHub →
channel: slack stale
## Summary - When `resolveThreadTsFromHistory()` fails (rate-limited, network error, etc.), the error was silently cached as `null` ("confirmed no thread_ts") for the full 60-second TTL - Every subsequent resolve() for that message during the TTL window returns the wrong answer, causing thread replies to lose their thread context and land in the main channel - Users reported ~10% of thread replies silently losing context (#12389) ## Root cause `resolveThreadTsFromHistory()` returns `undefined` on API failure (line 40). The caller converts it via `resolved ?? null` and caches the `null` value. `getCached` returns `null`, and `cached !== undefined` is true, so the cached `null` is treated as a confirmed "no thread_ts" result. ## Fix Return a `{ ok, threadTs }` result object from `resolveThreadTsFromHistory()` so callers can distinguish "API succeeded, no thread_ts found" (cache it) from "API call failed" (skip cache, retry next time). ## Test plan - [x] Added regression test: API failure on first call does not pollute cache; second call retries and succeeds - [x] Existing cache test still passes - [x] All existing tests pass Closes #12389 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> <!-- greptile_comment --> <h2>Greptile Overview</h2> <h3>Greptile Summary</h3> This PR changes Slack thread timestamp resolution to avoid caching negative results when the Slack `conversations.history` API call fails (rate limits/network errors). `resolveThreadTsFromHistory` now returns an `{ ok, threadTs }` result so callers can distinguish a successful lookup with no `thread_ts` (cache `null`) from a failed API call (skip cache), preventing a 60s cache pollution window that can cause thread replies to post to the main channel. A regression test was added to verify that a rejected API call does not populate the cache and that a subsequent call retries and succeeds, while the existing cache-hit behavior remains covered. <h3>Confidence Score: 5/5</h3> - This PR is safe to merge with minimal risk. - The change is narrowly scoped to Slack thread_ts resolution and fixes a concrete cache-pollution bug by distinguishing API failure from a legitimate “no thread” result. Types and control flow remain straightforward, and the added regression test directly covers the failure-then-retry scenario that motivated the fix. - No files require special attention <!-- greptile_other_comments_section --> <!-- /greptile_comment -->

Most Similar PRs