Skip to content

fix(platform-apple): do not start an alert action retry the remaining budget cannot hold - #4123

Open
pcbeingused333 wants to merge 6 commits into
callstack:mainfrom
pcbeingused333:fix/alert-wait-last-read
Open

pcbeingused333 wants to merge 6 commits into
callstack:mainfrom
pcbeingused333:fix/alert-wait-last-read

Conversation

@pcbeingused333

@pcbeingused333 pcbeingused333 commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Refs #4018. The wait path described there is already covered on main by #4187 (awaitAppleAlert now sleeps out the remainder when it is at most one poll interval), so after the rebase this PR keeps only the matching gap in actOnAppleAlert.

Root cause

actOnAppleAlert sleeps ALERT_POLL_INTERVAL_MS after each typed absence and starts another runner call with remainingBudgetMs(...), which returns Math.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, so accept/dismiss reports 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

@vercel

vercel Bot commented Oct 31, 2026

Copy link
Copy Markdown

@pcbeingused333 is attempting to deploy a commit to the Callstack Team on Vercel.

A member of the Team first needs to authorize it.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread packages/platform-apple/src/alert.ts Resolved
@thymikee

Copy link
Copy Markdown
Member

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 open --launch-url route (3000 ms window), which is a device-facing dispatch change (alert.ts#L109). The fake-timer tests with a mocked runner cannot show that the runner stops hitting its main-thread timeout and that the probe stops reporting alert-read. The failure was intermittent (seen twice on iPhone 17e, iOS 27.0), so mocks cannot confirm it is gone. Please run open <app> --launch-url <url whose scheme the app owns> on an iOS simulator where no "Open in" prompt appears, about 10 times to cover the last-poll window. In every run, launchConfirmationProbeFailure must be absent, launchConfirmation must be undefined, and there must be no alert-read warning. Also run alert wait --timeout 3000 on a quiet screen. It must end with the typed "alert wait timed out" absence (ALERT_NOT_FOUND), not a main-thread timeout. Record the device, source SHA and commands in the PR, or say there was no device available.

Not blocking, take or leave: the last read now starts about 500-800 ms before the end of the window, so awaitAppleAlert can finish roughly 2.5 s into a 3 s wait and miss an alert that appears in that tail (fine to accept, but worth a line in the PR body or alert wait docs); MIN_ALERT_READ_BUDGET_MS could live next to the other alert budgets in alert-contract.ts; the loop now has two budget checks plus a clock re-read, which one nextReadBudgetMs(start, windowMs): number | undefined could replace; and the new tests sit between the "whole point of typing absence" comment and the three tests it describes.

The two open inline threads still apply: the budgets.slice(1) test shape is a non-blocking nit, so assert budgets.length >= 2 first, as the wait test does. The macOS timeoutMs thread does not apply: the guard targets the iOS runner's main-thread timeout, which has no macOS helper equivalent, and bounding macOS helper reads is a separate gap. Please resolve it as out of scope.

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.

@pcbeingused333

Copy link
Copy Markdown
Contributor Author

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.

@thymikee

thymikee commented Nov 2, 2026

Copy link
Copy Markdown
Member

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 open --launch-url on an iOS simulator to confirm the alert read no longer times out, which we will pick up on our side.

@thymikee

thymikee commented Nov 4, 2026

Copy link
Copy Markdown
Member

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.
@pcbeingused333
pcbeingused333 force-pushed the fix/alert-wait-last-read branch from 13f2392 to f104c7d Compare November 5, 2026 00:52
@pcbeingused333 pcbeingused333 changed the title fix(platform-apple): do not start an alert read the remaining budget cannot hold fix(platform-apple): do not start an alert action retry the remaining budget cannot hold Nov 5, 2026
@pcbeingused333

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. Since #4187 already covers the wait path (awaitAppleAlert), this PR now keeps only the matching gap in actOnAppleAlert: no further runner read is started when the remaining budget is below 500 ms. The title and description are updated to match (now Refs #4018 instead of Fixes).

This comment was drafted with Claude Code.

@thymikee

thymikee commented Nov 5, 2026

Copy link
Copy Markdown
Member

Thanks for the rebase. At f104c7d the conflict is gone and CI is green. Since #4187 covers the open --launch-url wait route, what remains is the guard in alert.ts: alert accept and alert dismiss no longer send the last retry in the 2 s window when under 500 ms remain. The code reads correctly. The open item is live evidence for this device-facing change: the only test mocks runAppleRunnerCommand with fake timers, and the hosted Apple device lanes do not run for fork PRs, so we do not yet know how the runner answers a short-deadline alert command or whether the guard drops an alert that appears in the last 500 ms.

A maintainer iOS simulator run at f104c7d is the next step. In a quiet app with no alert, run alert accept and alert dismiss about 10 times each; every run should end with ALERT_NOT_FOUND and the scoped-snapshot hint (not ALERT_UNRESOLVED or MAIN_THREAD_TIMEOUT), and the runner log should show each alert command's timeoutMs at 500 or more. Then trigger a permission alert about 1 s after the command starts; alert accept must still accept it. The same loop on main would show whether the late-retry failure reproduces on this route.

Could the guard reuse the rule #4187 added? In awaitAppleAlert a read is skipped when remainingMs <= ALERT_POLL_INTERVAL_MS (300 ms). The act loop could do the same after its sleep, which removes MIN_ALERT_READ_BUDGET_MS and canHoldRead. If the live run shows 300 ms is not enough for the act route, one named floor in alert-contract.ts could serve both loops; today the same runner command gets two different minimum budgets with no recorded reason.

Not blocking, take or leave: add assert.ok(budgets.length >= 2) in alert.test.ts, so the budgets.slice(1) check cannot pass when the act loop never retries. That is also what the open test thread asks. The macOS timeoutMs thread does not apply: the guard only decides whether a retry starts, and macOS has no runner main-thread timeout to avoid, so it can be resolved.

…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>

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread packages/platform-apple/src/alert.ts Resolved
@thymikee

thymikee commented Nov 5, 2026

Copy link
Copy Markdown
Member

Thanks for the update. The code at eacd5a8 now fixes the earlier test gap: alert.test.ts now asserts budgets.length >= 2 before it checks each retry budget. The change itself looks right, but it is not ready to merge yet, because there is no live device run for the new guard.

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 alert accept and alert dismiss on an iOS simulator at eacd5a8 (pnpm build, pnpm build:xcuitest, pnpm clean:daemon) on the XCTest runner route. In a quiet app with no alert, about 10 runs of each must end with ALERT_NOT_FOUND and the scoped-snapshot hint, never ALERT_UNRESOLVED or MAIN_THREAD_TIMEOUT. The runner log must show every alert command with timeoutMs above 300, and no read started in the last 300 ms. Then trigger a permission alert about 1 s after the command starts, and confirm it is still accepted. Please record the device, source SHA and commands in the PR. Running the first check on main as well would show that the late-retry failure reproduces.

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 budget > ALERT_POLL_INTERVAL_MS would keep it in step with the contract. The PR body also still describes the old MIN_ALERT_READ_BUDGET_MS 500 ms design, so the Fix and Tests sections could say that a retry starts only with more than 300 ms left.

Earlier review: #4123 (comment)

@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member

Live run at eacd5a8 on iPhone 17 Pro simulator, iOS 27.0 (Xcode 27.1), XCTest runner route. Built with pnpm build and pnpm build:xcuitest, fresh daemon in a private state dir. Control: origin/main at c96a2d5, built the same way, own state dir.

1. Quiet app, no alert (Settings open), 10× alert accept + 10× alert dismiss

agent-device alert accept|dismiss --json --platform ios --udid <udid> --session <s>

  • eacd5a8: 20/20 ended ALERT_NOT_FOUND, with hint "If the permission sheet is visible in snapshot or screenshot but alert reports no alert, take a scoped snapshot around the visible button label and use press @ref." No ALERT_UNRESOLVED, no MAIN_THREAD_TIMEOUT. Each run made 4 runner reads in ~1.75 s. One run made a 5th read.
  • main: 20/20 ended ALERT_NOT_FOUND with the same hint. But every run made a 5th read with only ~215-293 ms left.

2. Read budgets (timeoutMs)

The runner log does not print timeoutMs. It logs only COMMAND_ACCEPTED command=alert commandId=.... So I worked out each read's budget from the request log: 2000 ms minus the time between request start and each ios_runner_command_send. Request start is a little before the retry loop starts, so these numbers are a lower bound.

  • eacd5a8, 61 retry reads: budgets ~1480-1570, ~1025-1150, ~590-725 ms. The single 5th read started at 1700 ms (budget ≈300 ms by this lower bound, so just above 300 ms by the guard). It returned ALERT_NOT_FOUND in 119 ms. No read started with clearly ≤300 ms left.
  • main: every run started a read in the last 300 ms (≈215-293 ms left). The failure reproduces on main in a quiet screen: 3 of 34 quiet calls ended MAIN_THREAD_TIMEOUT. Example: a 6th read started at 1969 ms with ≈31 ms left. The runner logged MAIN_THREAD_WORK_ABANDONED operation=command_execution slice=0.0 and the command ended MAIN_THREAD_TIMEOUT with no alert on screen. Two more the same way, with a 6th read at 2003-2004 ms.

3. Real permission alert that appears about 1 s after the command starts

Safari page on localhost that calls navigator.geolocation.getCurrentPosition when the host signals it. A new origin (port) for each run, so a fresh "“localhost” Would Like to Use Your Location" prompt appears 1.0-1.3 s after the daemon request starts.

  • eacd5a8: 3/3 late runs ended MAIN_THREAD_TIMEOUT, not accepted. The read that sees the prompt starts at 1.2-1.7 s with 300-760 ms left. The runner abandons it (slice=0.4 / 0.8). In one run the "Allow" tap was dispatched while the work drained, but the command still failed. In the other two the prompt stayed on screen. One earlier run (Safari system prompt, read with ≈1440 ms left) did the same: it tapped "Allow While Using App" and then failed with MAIN_THREAD_TIMEOUT (slice=1.5).
  • main: 3/3 late runs failed the same way (MAIN_THREAD_TIMEOUT, slice=0.7).
  • Alert already on screen when the command starts, eacd5a8: alert accept → accepted (several times), alert dismiss → dismissed.

So the guard removes the very late reads that cause the quiet-screen MAIN_THREAD_TIMEOUT on main. But a permission prompt that appears about 1 s into the 2 s window is still not accepted, on this head and on main. On this simulator, finding and tapping the alert takes more than 700 ms and sometimes more than 1.4 s. So 301 ms (and the current retry budget in general) is not enough for an alert that arrives late. This matches the open P1 thread on the retry floor.

Not covered: no runner log line with timeoutMs (the runner does not print it, so budgets above come from the daemon request log). Physical devices were not tested.

@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member

The live simulator run at eacd5a8 (#4123 (comment)) shows the quiet-screen fix works: 20 of 20 alert accept and alert dismiss calls end ALERT_NOT_FOUND, while main hit MAIN_THREAD_TIMEOUT in 3 of 34. It also answers the open P1 thread on the retry floor (#4123 (comment)): a permission prompt that appears about 1 s into the command still ends MAIN_THREAD_TIMEOUT (3 of 3, same as main), because the read that sees it starts with only about 300 to 760 ms left. Please raise one named floor in alert-contract.ts for both alert loops, enough for a late alert to be read and acted on, and add a test for it.

…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>

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread packages/platform-apple/src/alert.ts Resolved
thymikee and others added 2 commits November 7, 2026 14:47
…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>
@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member

For the Chief of Staff: the open P1 ask (one named floor for both alert loops) is done on this branch at 2ab8574.

Change

  • ALERT_MIN_READ_BUDGET_MS = 1_500 in alert-contract.ts. awaitAppleAlert and actOnAppleAlert start no retry read with that much budget or less left. It replaces the poll-interval floor, which is now only the sleep between reads. 1.5 s comes from the live run: the read that saw a late prompt needed up to about 1.4 s.
  • Backend split, from a Cubic P2: the floor exists for the XCTest runner's main-thread watchdog. The macOS helper takes no budget, so it keeps the poll-interval floor.

Tests

  • Act: the existing retry-budget test now asserts every retry read has more than 1.4 s. It failed with the old floor (retry started with only 1400 ms) and passes now.
  • Wait: the "admits another read" test is derived from the constant. A new macOS wait test keeps reading until a poll interval remains. It failed with a single 1.5 s floor.
  • Two existing tests had to move with the floor: the 900 ms unresolved-prompt wait is now 3 s, and the daemon alert dismiss retry test now expects one retry instead of two.

Receipt: pnpm check:affected --run --head 2ab8574bbd8582ee654cb1f8dcc34179391a823c passed, exactCommit=true, receipt run-8aVySV (in the worktree's .tmp/validation/). An earlier head, 51cb6c6, also passed (run-Kec1YI).

Live result: not run. The late-alert case needs a fresh build:xcuitest, the geolocation page, and several runs, so I did not call it quick. Unit tests do not show whether a late prompt is accepted. They show that no read starts with 1.5 s or less left, which is the condition behind the MAIN_THREAD_TIMEOUT.

Trade-off for the maintainer:

  • With ALERT_ACTION_RETRY_MS at 2 s, a 1.5 s floor leaves room for retries only in about the first 0.5 s. A prompt that appears about 1 s into alert accept or alert dismiss should now end with typed ALERT_NOT_FOUND and the scoped-snapshot hint, not MAIN_THREAD_TIMEOUT. That is my expectation from the fake-timer test, not a live result. It would not be accepted.
  • To accept a prompt that late, the action window has to grow (about 3 s or more). That is a separate constant, so I left it unchanged. The floor also shortens the tail of a short runner alert wait: --timeout 3000 stops polling about 1.5 s early.

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>
@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member

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 ALERT_MIN_READ_BUDGET_MS comment to the real guarantee: it gates retry reads only. Gate: pnpm check:affected --run --head 4eba55dfa90239c57299c84edfacf1c2e20d1007 passed, exactCommit=true, receipt run-9l6UY7. All threads are resolved.

@thymikee

thymikee commented Nov 7, 2026

Copy link
Copy Markdown
Member

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 budgets.length assertion are fixed in code. Two problems remain, so this is not ready yet. CI is green (11 checks), but the hosted Apple device lanes do not run for this fork PR, so no check exercises the XCTest runner alert route this diff changes. There are no conflicts.

The 1500 ms floor now also applies to the wait path (alert.ts:99-103), but the issue scope was narrowed to actOnAppleAlert, because #4187 already owns the wait path with the poll-interval rule. On iOS, after the first typed absence (about 120 ms), a wait of roughly 1.8 s or less sees at most 1500 ms remaining on its next turn, sleeps out the rest and stops. So alert wait 1000 is a single read, and a prompt that appears 300 ms in is reported as alert wait timed out. For the documented alert wait 3000 and the 3 s launch-confirmation probe, the last read starts near 1.5 s, so the second half of the window is never observed. #4187 left a blind spot of at most 300 ms; this makes it 1500 ms. The rule the wait path must satisfy: a wait whose window is longer than one poll interval keeps polling until less than one poll interval remains, for every timeout. Please keep awaitAppleAlert on the #4187 rule, apply the runner floor in actOnAppleAlert only, and revert the three retargeted wait tests (the 900 to 3000 change at alert.test.ts:363 and the 4_399/301 case), which pin the new floor instead of pinning that a short wait still polls. If a larger wait floor is wanted, it needs its own issue and a rule that does not blind waits shorter than the floor.

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 pnpm build, pnpm build:xcuitest and pnpm clean:daemon: (a) on a quiet Settings screen, 10x alert accept and 10x alert dismiss should all end in ALERT_NOT_FOUND with the scoped-snapshot hint, none in ALERT_UNRESOLVED or MAIN_THREAD_TIMEOUT, with the read count per run and the budget of each read (from the ios_runner_command_send offsets) showing no retry read under 1500 ms; (b) a localhost geolocation prompt raised about 0.3 s and about 0.8 s after the daemon request starts, with accepted vs ALERT_NOT_FOUND recorded for each and main as the control; (c) once the wait path is fixed, alert wait 1000 with a prompt at about 0.5 s should return it. Please record device, SHA and commands in the PR, and update the PR body, which still describes a 500 ms floor. I did not run the daemon or a simulator myself; the read counts above come from tracing the code against your earlier timings (about 120 ms reads, 300 ms sleep).

Could the floor live in the act loop only, as the issue now says? In actOnAppleAlert one runner-only constant (ALERT_MIN_READ_BUDGET_MS) would do, skipped on macOS where the helper takes no budget, or the leg returned by runAppleAlert could carry its own floor so the existing backend split is the only macOS check. minReadBudgetMs would then have no wait use, and the macOS wait test and the retargeted wait tests would go away. Separately, should ALERT_ACTION_RETRY_MS (2000) be raised? With a 1500 ms floor it leaves room for one retry. What needs deciding first is the intended act retry horizon: keep 2 s and accept one retry, or raise the window. Please note the choice in the PR or #4018.

Not blocking, take or leave: the act test asserts budget > 1_400 and budget > ALERT_MIN_READ_BUDGET_MS for the same value, so the first line can go with its comment, and a macOS act case that retries while more than a poll interval remains would pin the backend split for act; the doc above actOnAppleAlert could also state the effective retry horizon, since the post-sleep break leaves the loop condition nearly dead on iOS.

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 awaitAppleAlert and post live evidence at this head for the act loop (quiet 20/20 plus early and late prompt arrivals) and for a short alert wait.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants