Repository navigation
fix(platform-apple): do not start an alert action retry the remaining budget cannot hold - #4123
pcbeingused333 wants to merge 6 commits into
Conversation
|
@pcbeingused333 is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
The change looks right in code at 13f2392, but it needs live device evidence before merge. The guard changes when alert reads reach the iOS XCTest runner on the Not blocking, take or leave: the last read now starts about 500-800 ms before the end of the window, so The two open inline threads still apply: the The only failing check is the Vercel deploy authorization for the fork. It builds the website and does not touch this code, so it is likely unrelated. No other check ran on this head, so the test and gate lanes have not run, and the lint, format, tsc and test results in the PR body are unconfirmed. I did not run the unit tests, and I confirmed the red-before-fix claim only by replaying the budget arithmetic against the old loop. Before merge, the live simulator evidence above is needed, and a maintainer must authorize the real CI lanes for this fork head. |
|
Thanks for the detailed steps. I don't have access to macOS or an iOS simulator, so I can't produce that live evidence. I'll leave the code here as is: if someone on your side with a simulator can run the check, the change is ready; otherwise feel free to close it. |
|
Thanks, that makes sense. Nothing more is needed from you here. The code at 13f2392 still reads right to me, and the remaining step is a maintainer run of |
|
This conflicts with main now. Please rebase it so the remaining maintainer simulator run can happen on the rebased head. |
… budget cannot hold The alert act loop slept the poll interval and then started another runner read with whatever budget was left, which could be a few milliseconds. The runner answers such a read with a main-thread timeout, which is not a typed absence, so a quiet screen was reported as a failed read. Stop once the remaining budget cannot hold a read, so the window ends with the last typed absence. The wait loop already guards this on main.
13f2392 to
f104c7d
Compare
|
Rebased onto current main. Since #4187 already covers the wait path ( This comment was drafted with Claude Code. |
|
Thanks for the rebase. At f104c7d the conflict is gone and CI is green. Since #4187 covers the A maintainer iOS simulator run at f104c7d is the next step. In a quiet app with no alert, run Could the guard reuse the rule #4187 added? In Not blocking, take or leave: add |
…ion retries Drops MIN_ALERT_READ_BUDGET_MS and canHoldRead: the act loop now skips a retry when no more than ALERT_POLL_INTERVAL_MS remains, like awaitAppleAlert. The test also asserts the loop retried at least once. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
|
Thanks for the update. The code at eacd5a8 now fixes the earlier test gap: alert.test.ts now asserts The one reported check passes. The hosted Apple device lanes did not run for this fork PR, so CI does not exercise the runner alert route that this guard changes. Before merge, please run On the open threads: the P1 thread on the retry floor (#4123 (comment)) still applies. A retry can start with 301-499 ms left. That matches the floor used by awaitAppleAlert, so the live run above decides whether 301 ms is enough. If it is not, please raise one named floor in alert-contract.ts for both loops. Two threads can be resolved. The macOS timeout thread (#4123 (comment)) does not apply: the guard only decides whether another retry starts, and the dropped per-call timeout is already on main and out of scope. The thread on the retry count assertion (#4123 (comment)) is fixed at this head. Not blocking, take or leave: alert.test.ts:172 asserts the literal 300, so importing ALERT_POLL_INTERVAL_MS from @agent-device/contracts/alert-contract and asserting Earlier review: #4123 (comment) |
|
Live run at eacd5a8 on iPhone 17 Pro simulator, iOS 27.0 (Xcode 27.1), XCTest runner route. Built with 1. Quiet app, no alert (Settings open), 10×
2. Read budgets (timeoutMs) The runner log does not print
3. Real permission alert that appears about 1 s after the command starts Safari page on localhost that calls
So the guard removes the very late reads that cause the quiet-screen Not covered: no runner log line with |
|
The live simulator run at eacd5a8 (#4123 (comment)) shows the quiet-screen fix works: 20 of 20 |
…ind and press a late prompt Both alert loops skipped a read with no more than one poll interval left. A live run showed a permission prompt appearing about 1 s into the command still ended MAIN_THREAD_TIMEOUT: the read that saw it started with 300-760 ms left, and a read needs up to about 1.4 s to find and press the button. Replace the shared poll-interval floor with one named ALERT_MIN_READ_BUDGET_MS (1.5 s) in the alert contract, used by awaitAppleAlert and actOnAppleAlert. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Re-trigger cubic
…ad floor Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…helper The 1.5 s floor exists for the XCTest runner's main-thread watchdog. The macOS helper takes no budget, so it keeps reading until one poll interval remains. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
For the Chief of Staff: the open P1 ask (one named floor for both alert loops) is done on this branch at 2ab8574. Change
Tests
Receipt: Live result: not run. The late-alert case needs a fresh Trade-off for the maintainer:
Thank you, @pcbeingused333, for the original guard and the follow-up. Your commits are intact on the branch. |
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Follow-up to Cubic review 5812500105, which reviewed 3aa1507: both findings were already fixed in 2ab8574 (macOS keeps the poll-interval floor, with a test that failed before the split). 4eba55d only rewords the |
|
Thanks for the update. At 4eba55d the retry floor is one named constant, and the earlier threads on the 301-499 ms retries, the macOS floor, the contract comment and the The 1500 ms floor now also applies to the wait path (alert.ts:99-103), but the issue scope was narrowed to The only live run (comment 6702799288) is at eacd5a8 with a 300 ms floor. At this head the floor is 1500 ms inside a 2000 ms act window (alert.ts:159). The loop can make at most one retry, and only if the first read returns within about 200 ms, so a slow first read means no retry. That differs from the behavior validated before (4 reads in about 1.75 s). A prompt arriving 0.3-0.9 s into the window could be pressed on main, but no read starts after about 0.7 s here, so that arrival window shrinks, and the "late prompt is acted on" goal behind the floor is not shown. Please re-run on an iOS simulator at this head after a fresh Could the floor live in the act loop only, as the issue now says? In Not blocking, take or leave: the act test asserts On the other open threads: the P2 thread on the macOS floor, P3 thread on the contract comment, P1 thread on short retries and P2 thread on the budgets assertion are addressed in code and can be resolved once the points above are settled; the P1 one also depends on the live run. The P2 on the dropped macOS timeout does not apply: that behavior is on main already, and the guard only decides whether another retry starts. Before merge, please take the 1500 ms floor out of |
Refs #4018. The wait path described there is already covered on main by #4187 (
awaitAppleAlertnow sleeps out the remainder when it is at most one poll interval), so after the rebase this PR keeps only the matching gap inactOnAppleAlert.Root cause
actOnAppleAlertsleepsALERT_POLL_INTERVAL_MSafter each typed absence and starts another runner call withremainingBudgetMs(...), which returnsMath.max(1, remaining). Nothing checks that the remainder can hold a read, so the last retry can start with ~200 ms. The runner cuts it off with its own main-thread timeout, which is not a typed absence, soaccept/dismissreports a read failure instead of "no alert" plus the scoped-snapshot fallback hint.Fix
After the sleep, if the remaining budget is below
MIN_ALERT_READ_BUDGET_MS(500 ms), no further retry is started and the last absence is thrown with the fallback hint. Real read failures still propagate as themselves. The 500 ms floor is a chosen value (the runner contract defines no minimum read time); it is a single constant.Tests
One new test in
packages/platform-apple/src/__tests__/alert.test.ts(act, typed absences, fake timers): no retry starts with under 500 ms. It fails on main (retry started with 200 ms) and passes with the fix; the file's 25 tests pass, and oxlint and oxfmt are clean on the touched files.This change was prepared with an AI coding assistant (Claude).
🤖 Generated with Claude Code