Skip to content

fix(timeouts): prove the longpress request envelope with a worst-case ledger - #3937

Open
airowe wants to merge 12 commits into
callstack:mainfrom
airowe:test/longpress-envelope-ledger-3326
Open

airowe wants to merge 12 commits into
callstack:mainfrom
airowe:test/longpress-envelope-ledger-3326

Conversation

@airowe

@airowe airowe commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Builds on #3936 (merged as a9fc9a19f); main is merged in, so the diff is this change alone (5 files).

The longpress envelope (210 s) rested on a hand sum. test/integration/provider-scenarios/android-longpress-ledger.test.ts applies the #2879 ledger pattern to longpress 195 320 120000 on the Android cold-helper route:

  • A fake AndroidAdbProvider charges each call its own timeoutMs on a Date.now-spied clock, and fails on any call with no bound.
  • Observation dumps carry no focus line, so every variant is walked.

The worst branch costs 289,989 ms: checksum mismatch, an install refused as INSTALL_FAILED_UPDATE_INCOMPATIBLE, uninstall, reinstall, then a one-shot hold.

Steps Bound
version read 5 s
pm path, pull 2 × 10 s
install, uninstall, install 3 × 30 s
gesture 135 s
dialog and foreground observations 4 × 10 s

Adding the 30 s margin, the envelope rises to 320 s on every platform. The hold cap is now LONG_PRESS_DURATION_MAX_MS, read by the validator and the test.

Not charged: stopping a running snapshot-helper session waits on host-side timers (snapshot-helper-session-lifecycle.ts, ≤ 14 s). The test header documents it. #3814 edits command-descriptor-timeout-policy.test.ts too.

Closes #3326

Validation

  • pnpm check:affected --run --head 5e298d5a3 (main merged in): passed, exactCommit=true (base c48d27a81).
  • Negative control: envelopeMs: 319_000 fails ("289989ms + 30000ms margin exceeds the 319000ms envelope").
  • Timeout policy only, so no device run.

🤖 Generated with Claude Code

airowe and others added 2 commits October 23, 2026 22:22
…ecks

The blocking-dialog and foreground observations ran `adb shell dumpsys`
with no timeout before and after every Android interaction, so a hung
read waited for the client envelope and reset the daemon. Each dump read
now carries a 10 s bound. A timed-out read counts as unanswered, the same
`unknown` a read that printed nothing produces, so a slow probe still
never refuses the command; the reader then skips its remaining variants,
so one observation costs at most one bound.

This unblocks the longpress envelope ledger in callstack#3326, which cannot prove
an envelope while these reads are unbounded.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… ledger

The longpress envelope (210 s) rested on a hand sum that nothing
checked. A provider-integration ledger test now drives the Android
cold-helper route at the maximum hold through the daemon with a fake adb
provider that charges each call its own timeout on a virtual clock. The
slowest branch (checksum mismatch, an install refused as
INSTALL_FAILED_UPDATE_INCOMPATIBLE, uninstall, reinstall, one-shot hold)
costs 279,990 ms; with the 30 s daemon-result margin the envelope must be
at least 309,990 ms, so it rises to 310 s and its comment points at the
test. The 120 s hold cap becomes LONG_PRESS_DURATION_MAX_MS, read by both
the validator and the test.

Closes callstack#3326

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@thymikee

Copy link
Copy Markdown
Member

This PR is ready at 3f9b9cf, and I found nothing that needs to change before merge. All 10 checks pass, and there are no conflicts. I did not run the ledger test or its negative control. The 279,990 ms figure and the dumpsys count come from the PR body and my own recomputation of the sum.

Not blocking, and you can take or leave these: (1) The fake exec in android-longpress-ledger.test.ts answers the first dump variant of every observation, so the ledger charges one 10 s read each. The real route can walk more variants when one answers without a focus line, and each extra variant adds up to 10 s. Two blocking-dialog observations could then push the sum to about 330 s, over 310 s. The ledger should charge the maximum reads each observation can make, as the ios-fold ledger does for hinge reads. Alternatively, cap an observation at one bound in createDumpReader. (2) The old registry comment and the issue named handing off a running snapshot helper as part of the route. The ledger now drops it, and stopAndroidSnapshotHelperSession does not go through the provider seam the ledger records. One docstring sentence saying where that hand-off is bounded would close the gap. (3) In the same test, the header comment has two lines merged into one, and record() could read timeoutMs once into a const instead of using non-null assertions. (4) In registry.ts, the longpress envelope rises from 210 s to 310 s on every platform. A hung iOS, macOS or desktop longpress now waits 100 s longer before the reset, so the comment could say the envelope is Android-driven and applies everywhere.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 24, 2026
airowe and others added 2 commits October 23, 2026 23:30
A per-read bound still let one observation walk every dumpsys variant
that answered without a focus line, each spending up to its own 10 s.
All reads of one observation now draw on a single 10 s budget, so an
observation costs at most one bound however many variants it reads;
a spent budget or a timed-out read leaves the question unanswered.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The ledger's fake answered the first dumpsys variant of every
observation, so it charged one read each and never saw the foreground
check walk into a second observation. Observation dumps now answer
without a focus line, which sends each observation through every
variant its shared budget allows. The measured worst case rises to
289,989 ms, so the envelope rises from 310 s to 320 s.

Also: name the snapshot-helper hand-off the ledger cannot charge and
where it is bounded, say the Android-driven envelope applies to every
platform, read timeoutMs once in record(), and rewrap the header.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@airowe

airowe commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks. Taken in e10db9975, plus 8a582e78c on #3936:

  1. Variants: taken both ways.
    • fix(android): bound the dumpsys reads behind dialog and foreground checks #3936 now caps each observation at one 10 s budget, shared across all its variants.
    • The ledger fake now answers observation dumps without a focus line, so every variant is walked. That exposed a fourth observation: the foreground check falls through to a second readAppState.
    • The worst case is now 289,989 ms, so the envelope is 320 s (the negative control fails at 319 s).
  2. Hand-off: the test header now names the snapshot-helper hand-off and its bound (host-side timers in snapshot-helper-session-lifecycle.ts, 1 s + 11 s + 2 s). The ledger can't charge it. I left the roughly 14 s out of the envelope rather than copy those constants into the test. Say if you'd rather fold it in.
  3. Cleanup: header rewrapped. record() reads timeoutMs once.
  4. Comment: the registry comment now says the envelope applies to every platform.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member

This is ready at e10db99. The earlier note on 3f9b9cf stays resolved, and I found nothing new that needs a change. All 10 checks pass on this commit, and merge order depends on #3936 landing first, then main merged into the branch. Not blocking, take or leave: in https://github.com/callstack/agent-device/blob/e10db99/packages/platform-android/src/observation.ts#L93, createDumpReader computes the deadline and the remaining time by hand with Date.now(), while five other platform-android modules use host-kit's Deadline for this. Deadline.fromTimeoutMs(DUMP_OBSERVATION_BUDGET_MS) with isExpired() and remainingMs() would match them. I did not run the ledger test, its negative control, or the observation tests locally. I recomputed the 289,989 ms sum and it matches yours. The live evidence for observation.ts comes from the #3936 body, which I read but did not reproduce, and no hung-dumpsys live run exists. The ~20 s uncharged live-session branch is my estimate from the constants, not a measured run.

airowe and others added 4 commits October 24, 2026 00:31
The budget started when the reader was created, so the helper focus
call that readBlockingDialog (and readAppFocus, for a shared reader)
makes first spent up to 2 s of the 10 s dump budget. The deadline now
starts at the first dump read.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Matches the other platform-android modules that bound a sequence of
steps, instead of computing the deadline and remainder by hand.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@airowe

airowe commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Taken. createDumpReader now uses host-kit's Deadline (f02edea8e on #3936, merged here as c962e7aef). The deadline is created lazily at the first dump read (31676642a, from the #3936 review), so focus calls before it no longer spend the budget. The ledger still measures 289,989 ms against the 320 s envelope. check:affected --head c962e7aef passed. The live-session hand-off is still uncharged, as the test header says.

🤖 Addressed by Claude Code

@thymikee

Copy link
Copy Markdown
Member

Thanks for the update. At c962e7a I found no problems in the change. It builds on the earlier review at e10db99 (#3937 (comment)), and the live-session hand-off stays uncharged in the ledger, as the test header says.

All 11 checks pass, including the ledger test that exercises this route. I did not re-run the ledger test or observation.test.ts myself, so the 289,989 ms sum is your figure, backed by green CI. The new vi.mock('./automation-helper.ts') now applies to every test in observation.test.ts. It returns undefined and CI is green, so I think it is fine, but I did not compare each earlier test against the unmocked helper.

There are no conflicts. The merge order is unchanged: #3936 lands first.

airowe and others added 2 commits October 24, 2026 01:35
isExpired() and remainingMs() read the clock separately, so a
millisecond rollover between them passed timeoutMs 0, which the exec
layer treats as no timeout and runs the dump unbounded. The reader now
reads the remaining budget once and uses it for both the check and the
timeout.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@thymikee
thymikee marked this pull request as ready for review October 24, 2026 06:15
@thymikee

Copy link
Copy Markdown
Member

The PR is ready at 376fddd. The change since c962e7a (the once-per-read clock fix in the Android dump reader and its unit test) is correct, and the earlier clean review still applies. I did not run observation.test.ts or the ledger test, so the mocked-clock check was a hand simulation. I also did not run it on a live Android device, since it is a pure-JS clock read and the same code is already on main from #3936. The live-session hand-off stays uncharged in the ledger, as the test header says. The two Smoke Tests jobs are still running and none have failed. I did not check whether they exercise Android observation. They need to finish green before merge. Not blocking, and you can take or leave it: main (a9fc9a1) already carries the squashed #3936, so rebasing onto main would drop the seven duplicate #3936 commits and leave only the ledger, registry and contract changes in the PR.

@airowe

airowe commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Merged main (a9fc9a19f, the squashed #3936) into the branch at 5e298d5a3 instead of rebasing, so the PR diff is now this change alone: 5 files. I'm keeping to merge commits because I don't force-push published branches. A squash merge of this PR gives the same result as a rebase. check:affected --run --head 5e298d5a3 passed. The first run hit a 5 s timeout in http-server-proxy.test.ts. That file has no diff against main and passed 3 of 3 runs on its own, and the full gate passed on rerun.

🤖 Addressed by Claude Code

@airowe

airowe commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

The red Smoke Tests job at 5e298d5a3 is the iOS phase. macOS passed.

  • Where it failed: in live-automation-scenario.ts:97. open fixture sheet and the following wait text "Automation sheet" both passed. Then click id="automation-close-sheet" returned selector_not_found (dispatched: no).
  • Earlier retry: a replay retried a gesture wait once (pan duration >=400ms) earlier in the same run.
  • Not from this PR: nothing in that path goes through longpress or this change. The PR's diff against main is the ledger test, the envelope, a constant with the same 120 s value, and two pins. Main at a9fc9a19f (this PR's base) passed the same workflow.

I can't rerun jobs from a fork. Could you rerun the iOS smoke job?

🤖 Addressed by Claude Code

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

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prove the longpress request envelope with a worst-case ledger test

2 participants