fix(lr-0827ba): make rate-limit auto-schedule arming state per-session#340
Conversation
…n localId Server-side scheduling (session.scheduledMessage) is already correctly per-session. But the rate_limit, rate_limit_usage, scheduled_message_queued, scheduled_message_sent, and scheduled_message_cancelled payloads sent to the client carried no session-identifying field, so a client tracking per-session UI state (added in a follow-up commit) had no way to correlate an incoming event with the session it actually belongs to during history replay or multi-session use. No client-side behavior change in this commit; localId is additive.
Root cause: scheduleDelayMs in input.js and the related rate-limit bookkeeping (rateLimitResetsAt, rateLimitResetTimer, rateLimitResetState, scheduled-message bubble text) were bare module-scoped variables. Since the SPA never reloads the page on project switch, all sessions across all projects shared this single set of variables - arming the auto-schedule in a second project silently overwrote/lost the first project's armed state. resetRateLimitState, called on every session/project switch, also nulled out the outgoing session's reset timer and armed delay, canceling in-progress background arming instead of leaving it running. Fix: new lib/public/modules/rate-limit-state.js is a DOM-free per-session state map keyed by session localId, mirroring the existing sessionDrafts precedent. Covers schedule-delay arming, rate-limit reset bookkeeping, and the scheduled-message-bubble text/resetsAt. input.js and app-rate-limit.js now read/write through this module instead of bare variables. Each session's background reset timer keeps running even when that session isn't focused, mirroring server-side session.scheduledMessage in lib/project.js which was already correct. A session's own background rate-limit event now arms/redraws using its own state regardless of which session is currently in view, via msg.localId stamped server-side in the preceding commit. app-messages.js's session_switched handler now calls restoreRateLimitStateForSession after resetClientState to redraw the incoming session's own armed/queued state - the redraw-on-switch-in hook - rather than relying on the previous unreliable chat-history-replay side effect. Trade-off: forgetSessionRateLimitState and clearSession exist for pruning a session's state when it is actually deleted, not merely switched away from, but are not wired into the delete_session flow in this diff. renderSessionList only receives the current project's session list, so pruning there risks cross-project ambiguity, and the existing sessionDrafts map, the established per-session-state precedent, has the same never-pruned-on-delete gap already. Named here rather than guessing at a wiring point. Regression tests: test/rate-limit-state.test.js covers per-session isolation of armed state, clearSession teardown including canceling the background timer, scheduled-message-bubble isolation, and session-id keying edge cases. Refs #328.
…redraw Source-text regression tests (matching the existing frontend-state-correlation-lr-fb49.test.js convention for DOM/live-server coupled modules that cannot be imported directly under plain Node): - lib/project.js and lib/sdk-message-processor.js stamp scheduled_message_queued/sent/cancelled and rate_limit/rate_limit_usage payloads with localId: session.localId. - app-messages.js forwards msg.localId through to addScheduledMessageBubble/clearScheduledMessage instead of assuming the event always belongs to the focused session. - session_switched calls restoreRateLimitStateForSession AFTER resetClientState, so the redraw lands after the DOM is cleared for the newly active session, not the outgoing one. - resetRateLimitState no longer nulls a session's background reset timer. - handleRateLimitEvent arms the event's own session (eventSessionId) rather than unconditionally the currently-focused one. Refs #328.
|
PEACHES — blocking Session cleanup gap: forgetSessionRateLimitState() is implemented but never called when a session is deleted. This causes per-session rate-limit state entries and their background reset timers (setTimeout) to leak indefinitely, accumulating memory over time as sessions are created/deleted. Details:
Mitigation: Call forgetSessionRateLimitState(sessionId) when the server confirms session deletion (likely via new session_deleted message or by cleanup in sidebar-sessions.js immediately after delete_session send with server confirmation). |
…te (PEACHES) PEACHES flagged a real defect on PR #340: forgetSessionRateLimitState()/ clearSession() correctly cancel a session's background rate-limit reset timer and drop its per-session state, but nothing called them on session deletion. sidebar-sessions.js sends delete_session over WS and the server deletes the session, but the client never learned WHICH session id disappeared -- session_list just silently omitted it -- so rate-limit-state.js's _bySession map and any pending reset-timer setTimeout leaked forever for every deleted session. Fix, server-side (lib/sessions.js): new broadcastSessionDeleted(localIds) sends { type: session_deleted, ids: [...] }, called from deleteSession() (single) and deleteSessionsBulk() (batch) -- the two places sessions are actually removed from the sessions Map. lib/project-loop.js's delete_loop_group handler (a third deletion path, used when a whole loop group is torn down) calls it too, since it already computes the exact sessionIds it deletes. Fix, client-side (app-messages.js): new session_deleted dispatch case calls forgetSessionRateLimitState() for every id in the payload. This mirrors the per-session server-authority pattern already used elsewhere in this PR (server stamps/broadcasts session identity, client reacts) rather than inventing client-side-only cleanup, and fully closes the leak PEACHES identified rather than deferring it again. Regression tests (test/session-deleted-rate-limit-cleanup-lr-0827ba.test.js): - lib/sessions.js: deleteSession and deleteSessionsBulk broadcast session_deleted with the exact deleted id(s), using a real createSessionManager + send spy (matches session-lifecycle-lr-e0de.test.js convention) -- no broadcast when nothing was actually deletable. - app-messages.js / project-loop.js: source-text checks confirming the dispatch wiring (DOM/live-WS coupled modules, matches frontend-state-correlation-lr-fb49.test.js convention). - rate-limit-state.js: end-to-end proof that clearSession (the delegate forgetSessionRateLimitState calls) actually cancels the pending timer and drops the entry, scoped to the deletion scenario PEACHES flagged. npm test: 956 tests, 953 pass, 3 fail (same pre-existing sdk-bridge-context-window-warn.test.js failures as before, confirmed baseline-only in the prior commit's testing). Refs #328, PR #340 PEACHES review.
|
PEACHES — clean (0 nits) Re-review of PR #340 (lr-0827ba / issue #328) — the blocking finding from the prior review has been fully resolved. Prior Finding (Blocking): forgetSessionRateLimitState() was implemented but never called on session deletion, causing memory/timer leaks (orphaned reset timers and per-session state entries). Fix Verification:
No new issues detected. Code follows AMoS craft rules, no hardcoded paths, no brand violations, client/server layer boundaries respected. |
|
BOBBIE -- clean Audited PR #340 (lr-0827ba / issue #328, head 2ecd916) -- per-session rate-limit auto-schedule arming state, plus the new session_deleted WS broadcast and localId stamping on scheduled-message/rate-limit payloads. Findings: none blocking, none nit. Scope verified: 11 files, +840/-50 (matches assignment), diffed base 3f25c02..head 2ecd916. Reasoning on the flagged surface:
Scanner results:
No rulebook-citable findings within the base..head diff. scanners_run: gitleaks (ok), semgrep (ok), osv-scanner (ok, no-diff-in-scope), trufflehog (unavailable)
|
|
Merged via clagentic-loadout v0.1.0
|
Fixes lr-0827ba / GitHub issue #328: rate-limit auto-schedule arming state was global to the browser tab instead of per-session.
Root cause
Fix
Trade-off (named, not silently shipped)
forgetSessionRateLimitState()/clearSession() exist in rate-limit-state.js for pruning a session state when it is actually deleted (not merely switched away from), but are not wired into the delete_session flow here. renderSessionList() only receives the current project session list, so pruning there risks cross-project ambiguity, and the existing sessionDrafts map (the established per-session-state precedent) has the same never-pruned-on-delete gap already. Flagging here rather than guessing at a wiring point outside this task scope.
Tests
npm test: 950 tests, 947 pass, 3 fail. The 3 failures are pre-existing and unrelated to this change -- all three are in test/sdk-bridge-context-window-warn.test.js (lr-1f7e cases 1, 3, 10), confirmed to fail identically on unmodified origin/main via a throwaway git worktree comparison before this diff was applied.
lr-0827ba
Closes #328