fix(slack): preserve active prefetch claims under cap - #325
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughSlack prefetch claims now commit only after history injection. Active claims remain protected when capacity is full. Completed claims can be evicted. Slack event paths now commit and release claims according to their lifecycle. ChangesSlack prefetch lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…context # Conflicts: # structure/str_func.md
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/slack/bot.ts`:
- Around line 241-245: Update handleSlackEnvelope’s post-prefetch-token flow to
use a try/finally that releases the acquired claim on every early return and
attachment-recovery exception. Track whether enqueueSlackIngress (or the
existing ownership-transfer path) accepted the token, and only skip release when
ownership was accepted; preserve normal processing and commit behavior
otherwise.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3549565e-0dc8-4f80-940f-062ff8a69ff6
📒 Files selected for processing (4)
src/slack/bot.tssrc/slack/thread-tracker.tsstructure/str_func.mdtests/unit/slack-thread-prefetch.test.ts
| await runSlackMessageEvent(event, target, text, signal, opts, () => { | ||
| prefetchCommitted = Boolean(opts.prefetchToken) && commitThreadPrefetch( | ||
| event.channel || '', event.thread_ts || '', opts.prefetchToken || 0, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Release claims on every pre-enqueue exit.
processSlackMessageEvent releases only claims that reach its finally. handleSlackEnvelope acquires prefetchToken before attachment recovery, then can return at Line 500 or Lines 503-511 without passing the claim to this function. An attachment-recovery exception has the same result.
These paths leave an uncommitted claim active. The new capacity logic cannot evict active claims. After 500 such threaded events, all later prefetch claims are declined until reset.
Wrap post-claim work in a try/finally. Release the token unless enqueueSlackIngress accepts ownership.
Proposed lifecycle guard
const prefetchToken = event.thread_ts
? claimThreadPrefetch(event.channel || '', event.thread_ts)
: 0;
+let prefetchHandedOff = false;
-const target = buildSlackTarget(event);
+try {
+const target = buildSlackTarget(event);
// ... attachment recovery and early-return paths ...
enqueueSlackIngress(slackIngressLaneKey(target), signal =>
processSlackMessageEvent(event, target, text, signal, {
prefetchToken,
// ...
}));
+prefetchHandedOff = true;
+} finally {
+ if (prefetchToken && !prefetchHandedOff) {
+ releaseThreadPrefetch(event.channel || '', event.thread_ts || '', prefetchToken);
+ }
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/slack/bot.ts` around lines 241 - 245, Update handleSlackEnvelope’s
post-prefetch-token flow to use a try/finally that releases the acquired claim
on every early return and attachment-recovery exception. Track whether
enqueueSlackIngress (or the existing ownership-transfer path) accepted the
token, and only skip release when ownership was accepted; preserve normal
processing and commit behavior otherwise.
1 participant
Scope
Narrow #316 follow-up discovered while auditing #317. The #317 conversation-context production wiring is already present on
dev; this PR does not duplicate it.At the 500-entry prefetch-claim cap, the prior eviction path could remove an active in-flight claim. A concurrent envelope for that same thread could then acquire a second claim and inject the same history twice.
Change
Verification
npm run typecheck: passednpm run build: passedstructure/verify-counts.sh: all 366 file-tree entries matched, 3 paths skipped; the script still exited 1 after the aggregate/public phase due the existing aggregate-parser behavior, so a full verify-counts pass is not claimedNo Slack scopes changed. Message delivery remains fail-open when no prefetch claim is available.
Summary by CodeRabbit
Bug Fixes
Tests