fix(server): re-snoozing a woken thread to the same wake time hides it again - #14299
vitalyiegorov wants to merge 1 commit into
Conversation
…t again A snooze to the same wake time kept the original snoozedAt, so a failure or completion that woke the thread stayed newer than the snooze and the thread never hid again. Every snooze now stamps fresh; retries are already deduplicated by commandId. Fixes pingdotgg#14298 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a narrowly scoped snooze lifecycle bug fix that synchronizes server and optimistic client timestamps, with a regression test and no schema, default, infrastructure, or sensitive-area changes. Its runtime impact is limited to correctly hiding a thread again when the user re-snoozes it. You can add or adjust custom eligibility rules. Learn more. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: pingdotgg/t3code/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe server now stamps fresh snooze and update times for every snooze command. The client’s optimistic update also stamps the current time. A test checks repeated snoozes to the same wake time. ChangesSnooze timestamp refresh
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Suggested reviewers: Merge Risk: ⚪ Minimal · up to Repeated snoozes now refresh the baseline so earlier failures no longer immediately wake the thread. No concrete merge-blocking risk is established; the possible cross-instance retry effect remains unconfirmed. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
What Changed
thread.snoozenow always stampssnoozedAtwith the current time. I removed the branch that kept the originalsnoozedAtwhen a thread was re-snoozed to the same wake time. It was in the server decider (apps/server/src/orchestration/decider.ts) and in its optimistic twin on the client (packages/client-runtime/src/state/threadCommands.ts). The diff removes more lines than it adds and changes no contract or schema.The existing decider test that asserted the old behavior now asserts the fix: a re-snooze to the same wake time gets a fresh
snoozedAt. Without the fix it fails.Why
Fixes #14298.
threadRaisedHandWhileSnoozedwakes a snoozed thread when a failure or a completion is newer thansnoozedAt. Once that happens, the user snoozes the thread again, usually with the same preset. Presets such as Tomorrow and Next week resolve to fixed clock times, so the new request carries the same wake time. The decider treated it as a duplicate and kept the oldsnoozedAt. The failure was therefore still "newer than the snooze", and the thread never hid again. The server accepted every retry and nothing changed.In the reported case, one Claude thread hit its usage limit after being snoozed and was re-snoozed six times in 12 minutes. Every attempt was accepted and none of them hid the thread.
A fresh stamp is the right meaning: re-snoozing a woken thread is the user saying "seen it, not now". The old branch protected against double-clicks and racing clients. Real retries are already deduplicated by
commandIdreceipts, and a double-click now only moves the stamp by milliseconds.This only covers re-snoozing. Whether completed work or a usage-limit failure should wake a snoozed thread at all (#6368) is a separate policy question, and this PR doesn't touch it.
Verification:
vp test runondecider.snoozed.test.ts,decider.active-order.test.ts,ProjectionPipeline.test.tsand client-runtimethreadCommands.test.tsandthreadSnoozed.test.ts: 100 passed. The new case fails without the fix.apps/serverandpackages/client-runtime.Checklist
Implemented with Claude Opus 5.5 and Claude Sonnet 5.5 in T3 Code (Claude Code harness).
🤖 Generated with Claude Code
Summary by CodeRabbit