fix(modal-infra): guard _current_prompt_task against overlap#80
Conversation
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Greptile OverviewGreptile SummaryThis PR fixes a race in |
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in modal-infra’s sandbox bridge where overlapping prompt commands could cause an older prompt task’s done-callback to clear _current_prompt_task after a newer prompt has started, which would break stop behavior.
Changes:
- Guard
_current_prompt_taskclearing so only the currently-tracked task can clear itself on completion. - Add a regression test to ensure an older prompt completion can’t wipe out a newer active prompt task.
- Minor test cleanup (import usage and cancellation handling).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/modal-infra/src/sandbox/bridge.py | Prevents stale prompt task completion from clearing _current_prompt_task for a newer prompt. |
| packages/modal-infra/tests/test_bridge_stop.py | Adds regression coverage for overlapping prompt tasks; small test cleanups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…odal verdict Two follow-ups to reef's review on ColeMurray#80: 1. The comment claimed "TTL is short by design — plans are decided in minutes, not hours" but the constant was 7 days. Drop to 24h and reword the comment so the intent matches. 2. The modal-driven verdict path (handlePlanApproveSubmission / handlePlanRejectSubmission) wasn't clearing the KV entry, so stale refs accumulated on every same-channel approval until the TTL expired. Add a clearPlanAwaitingKvRef helper called after the chat.update succeeds — also defends against a late-arriving cross-channel callback re-updating the message after the modal handler already settled it. The TTL now acts as a floor (sessions whose awaiting message is never acted on), not the primary lifecycle bound. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…odal verdict Two follow-ups to reef's review on ColeMurray#80: 1. The comment claimed "TTL is short by design — plans are decided in minutes, not hours" but the constant was 7 days. Drop to 24h and reword the comment so the intent matches. 2. The modal-driven verdict path (handlePlanApproveSubmission / handlePlanRejectSubmission) wasn't clearing the KV entry, so stale refs accumulated on every same-channel approval until the TTL expired. Add a clearPlanAwaitingKvRef helper called after the chat.update succeeds — also defends against a late-arriving cross-channel callback re-updating the message after the modal handler already settled it. The TTL now acts as a floor (sessions whose awaiting message is never acted on), not the primary lifecycle bound. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When two
promptcommands overlap, the older task’s done-callback could clear_current_prompt_taskafter a newer prompt has already started. That breaksstop, since_handle_stopcancels_current_prompt_task.This change only clears
_current_prompt_taskif the finished task is still the current one, and adds a regression test to ensure an older completion can’t wipe out a newer active prompt task.