Skip to content

fix: rate-limit notification retries for non-existent windows #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
UserAd merged 2 commits into from
Jan 15, 2026
Merged

fix: rate-limit notification retries for non-existent windows #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b596843
fix: rate-limit notification retries for non-existent windows
UserAd Jan 15, 2026
ddfb5f1
fix: check window existence before notification attempt
UserAd Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions internal/daemon/loop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ func (opts *LoopOptions) log(format string, args ...interface{}) {
// NotifyFunc is the function signature for notifying an agent.
type NotifyFunc func(window string) error

// WindowCheckerFunc is the function signature for checking if a window exists.
type WindowCheckerFunc func(window string) (bool, error)

// NotifyAgent sends a notification to an agent's tmux window.
// Notification protocol:
// 1. tmux send-keys -t <window> "Check your agentmail"
Expand Down Expand Up @@ -122,18 +125,19 @@ func NotifyAgent(window string) error {
func CheckAndNotify(opts LoopOptions) error {
if opts.SkipTmuxCheck {
// In test mode, skip actual notifications but still update flags
return CheckAndNotifyWithNotifier(opts, nil)
return CheckAndNotifyWithNotifier(opts, nil, nil)
}
return CheckAndNotifyWithNotifier(opts, NotifyAgent)
return CheckAndNotifyWithNotifier(opts, NotifyAgent, tmux.WindowExists)
}

// CheckAndNotifyWithNotifier performs a single notification cycle with a custom notifier.
// This allows for testing without actual tmux calls.
// When notify is non-nil, it will be called for each agent that should be notified.
// When windowChecker is non-nil, it will be used to verify window existence before notifying.
// The function handles two types of agents:
// - Phase 1: Stated agents (with recipient state in recipients.jsonl)
// - Phase 2: Stateless agents (mailbox but no recipient state)
func CheckAndNotifyWithNotifier(opts LoopOptions, notify NotifyFunc) error {
func CheckAndNotifyWithNotifier(opts LoopOptions, notify NotifyFunc, windowChecker WindowCheckerFunc) error {
opts.log("Starting notification cycle")

// =========================================================================
Expand Down Expand Up @@ -246,11 +250,27 @@ func CheckAndNotifyWithNotifier(opts LoopOptions, notify NotifyFunc) error {
continue
}

// Check if window exists before attempting notification
if windowChecker != nil {
exists, err := windowChecker(mailboxRecipient)
if err != nil {
opts.log("Error checking window existence for %q: %v", mailboxRecipient, err)
continue
}
if !exists {
opts.log("Skipping stateless agent %q: window does not exist", mailboxRecipient)
continue
}
}

// Send notification
if notify != nil {
opts.log("Notifying stateless agent %q", mailboxRecipient)
if err := notify(mailboxRecipient); err != nil {
opts.log("Notification failed for stateless agent %q: %v", mailboxRecipient, err)
// Mark as notified even on failure to rate-limit retries
opts.StatelessTracker.MarkNotified(mailboxRecipient)
opts.log("Marked stateless agent %q in tracker (after failure)", mailboxRecipient)
continue
}
opts.log("Notification sent to stateless agent %q", mailboxRecipient)
Expand Down
127 changes: 92 additions & 35 deletions internal/daemon/loop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func TestCheckAndNotify_NotifiesReadyAgentWithUnreadMessages(t *testing.T) {
SkipTmuxCheck: true,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -148,7 +148,7 @@ func TestCheckAndNotify_NoNotificationWhenNoUnreadMessages(t *testing.T) {
SkipTmuxCheck: true,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -183,7 +183,7 @@ func TestCheckAndNotify_SkipsWorkAgent(t *testing.T) {
SkipTmuxCheck: true,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -214,7 +214,7 @@ func TestCheckAndNotify_SkipsOfflineAgent(t *testing.T) {
SkipTmuxCheck: true,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -251,7 +251,7 @@ func TestCheckAndNotify_MixedStatuses(t *testing.T) {
SkipTmuxCheck: true,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -289,7 +289,7 @@ func TestCheckAndNotify_SkipsAlreadyNotified(t *testing.T) {
SkipTmuxCheck: true,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -318,7 +318,7 @@ func TestCheckAndNotify_UpdatesNotifiedFlagAfterNotification(t *testing.T) {
SkipTmuxCheck: true,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -527,7 +527,7 @@ func TestIntegration_FullNotificationCycle(t *testing.T) {
}

// Run CheckAndNotify
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotify failed: %v", err)
}
Expand Down Expand Up @@ -927,7 +927,7 @@ func TestStatelessNotification_AgentWithMailboxNoState(t *testing.T) {
StatelessTracker: tracker,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -965,7 +965,7 @@ func TestStatelessNotification_RespectInterval(t *testing.T) {
}

// First call: should notify (first time)
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand All @@ -974,7 +974,7 @@ func TestStatelessNotification_RespectInterval(t *testing.T) {
}

// Second call immediately: should NOT notify (interval not elapsed)
err = CheckAndNotifyWithNotifier(opts, mockNotify)
err = CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand All @@ -986,7 +986,7 @@ func TestStatelessNotification_RespectInterval(t *testing.T) {
time.Sleep(60 * time.Millisecond)

// Third call: should notify again (interval elapsed)
err = CheckAndNotifyWithNotifier(opts, mockNotify)
err = CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1021,7 +1021,7 @@ func TestStatelessNotification_NoUnreadMessages(t *testing.T) {
StatelessTracker: tracker,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1056,7 +1056,7 @@ func TestStatelessNotification_MultipleAgents(t *testing.T) {
StatelessTracker: tracker,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1111,7 +1111,7 @@ func TestStatelessNotification_StatedAgentTakesPrecedence(t *testing.T) {
StatelessTracker: tracker,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1179,7 +1179,7 @@ func TestStatelessNotification_TransitionToStated(t *testing.T) {
}

// First call: notified as stateless
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand All @@ -1195,7 +1195,7 @@ func TestStatelessNotification_TransitionToStated(t *testing.T) {
createRecipientState(t, repoRoot, "transitioning-agent", mail.StatusReady, false, now)

// Second call: agent is now stated, should be notified via stated logic
err = CheckAndNotifyWithNotifier(opts, mockNotify)
err = CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand All @@ -1213,7 +1213,7 @@ func TestStatelessNotification_TransitionToStated(t *testing.T) {
}

// Third call: already notified as stated, should NOT get notified again
err = CheckAndNotifyWithNotifier(opts, mockNotify)
err = CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1251,7 +1251,7 @@ func TestStatelessNotification_DaemonRestart_ImmediateEligibility(t *testing.T)
}

// First daemon: both agents notified
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand All @@ -1265,7 +1265,7 @@ func TestStatelessNotification_DaemonRestart_ImmediateEligibility(t *testing.T)
opts.StatelessTracker = tracker2

// Second daemon: agents should be immediately eligible again (fresh tracker)
err = CheckAndNotifyWithNotifier(opts, mockNotify)
err = CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1299,7 +1299,7 @@ func TestStatelessNotification_MailboxDirReadError(t *testing.T) {
}

// Should not return error even if mailbox dir doesn't exist
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
// ReadAllRecipients will fail first, so this is expected to return error
// But the system should handle it gracefully
if err == nil {
Expand All @@ -1313,7 +1313,8 @@ func TestStatelessNotification_MailboxDirReadError(t *testing.T) {
}

// T033: TestStatelessNotification_NotifyFailure (FR-015)
// Tests that notification failure doesn't mark agent as notified
// Tests that notification failure marks agent as notified to rate-limit retries
// This prevents infinite retry loops for non-existent windows
func TestStatelessNotification_NotifyFailure(t *testing.T) {
repoRoot := createTestMailDir(t)

Expand All @@ -1338,27 +1339,83 @@ func TestStatelessNotification_NotifyFailure(t *testing.T) {
}

// First call: notification fails
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
if notifyAttempts != 1 {
t.Errorf("Expected 1 notify attempt, got %d", notifyAttempts)
}

// Verify agent is NOT marked as notified in tracker (should retry on next call)
// Since notification failed, ShouldNotify should still return true
if !tracker.ShouldNotify("failing-agent") {
t.Error("Agent should still be eligible after notification failure")
// Verify agent IS marked as notified in tracker (rate-limited)
// This prevents infinite retry loops for non-existent windows
if tracker.ShouldNotify("failing-agent") {
t.Error("Agent should be rate-limited after notification failure")
}

// Second call: should retry since first failed
err = CheckAndNotifyWithNotifier(opts, mockNotify)
// Second call immediately: should NOT retry (rate-limited)
err = CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
if notifyAttempts != 1 {
t.Errorf("Expected still 1 notify attempt (rate-limited), got %d", notifyAttempts)
}

// Wait for interval to elapse
time.Sleep(60 * time.Millisecond)

// Third call: should retry after interval elapsed
err = CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
if notifyAttempts != 2 {
t.Errorf("Expected 2 notify attempts, got %d", notifyAttempts)
t.Errorf("Expected 2 notify attempts after interval, got %d", notifyAttempts)
}
}

// TestStatelessNotification_WindowNotExists
// Tests that non-existent windows are skipped without notification attempt
func TestStatelessNotification_WindowNotExists(t *testing.T) {
repoRoot := createTestMailDir(t)

// Create a stateless agent with unread messages
createUnreadMessage(t, repoRoot, "missing-window", "sender", "Hello!")

tracker := NewStatelessTracker(50 * time.Millisecond)

notifyAttempts := 0
mockNotify := func(window string) error {
notifyAttempts++
return nil
}

// Mock window checker that says window doesn't exist
mockWindowChecker := func(window string) (bool, error) {
return false, nil // Window does not exist
}

opts := LoopOptions{
RepoRoot: repoRoot,
SkipTmuxCheck: true,
StatelessTracker: tracker,
}

// Should skip notification because window doesn't exist
err := CheckAndNotifyWithNotifier(opts, mockNotify, mockWindowChecker)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}

// No notification should have been attempted
if notifyAttempts != 0 {
t.Errorf("Expected 0 notify attempts for non-existent window, got %d", notifyAttempts)
}

// Agent should NOT be marked as notified (so it can retry when window appears)
if !tracker.ShouldNotify("missing-window") {
t.Error("Agent should still be eligible when window doesn't exist")
}
}

Expand Down Expand Up @@ -1392,7 +1449,7 @@ func TestStatelessNotification_MailboxFileReadError(t *testing.T) {
}

// Should handle the bad mailbox gracefully and still notify good agent
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1435,7 +1492,7 @@ func TestStatelessNotification_RecipientsReadError(t *testing.T) {
}

// Should return error because ReadAllRecipients fails
err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err == nil {
// If we want to implement FR-017 (fallback to all stateless), we'd need to
// change the error handling in Phase 1 to continue to Phase 2
Expand Down Expand Up @@ -1509,7 +1566,7 @@ func TestLogging_ForegroundMode(t *testing.T) {
Logger: &logBuf,
}

err := CheckAndNotifyWithNotifier(opts, mockNotify)
err := CheckAndNotifyWithNotifier(opts, mockNotify, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1556,7 +1613,7 @@ func TestLogging_NoLoggerNoOutput(t *testing.T) {
}

// This should not panic even with nil logger
err := CheckAndNotifyWithNotifier(opts, nil)
err := CheckAndNotifyWithNotifier(opts, nil, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down Expand Up @@ -1593,7 +1650,7 @@ func TestLogging_SkipMessages(t *testing.T) {
Logger: &logBuf,
}

err := CheckAndNotifyWithNotifier(opts, nil)
err := CheckAndNotifyWithNotifier(opts, nil, nil)
if err != nil {
t.Fatalf("CheckAndNotifyWithNotifier failed: %v", err)
}
Expand Down