Skip to content

fix(android): record the instrumentation result when the snapshot helper exits without output - #4021

Merged
thymikee merged 18 commits into
mainfrom
maint/4005-android-helper-exit-diagnostics
Oct 29, 2026
Merged

thymikee merged 18 commits into
mainfrom
maint/4005-android-helper-exit-diagnostics

Conversation

@thymikee

@thymikee thymikee commented Oct 28, 2026 •

Copy link
Copy Markdown
Member

Summary

When the Android snapshot helper exits without parseable output, the error and request log carry a typed instrumentation detail (#4005): exit status and signal, result and status codes, INSTRUMENTATION_ABORTED, the INSTRUMENTATION_FAILED component, and capped shortMsg/longMsg/Error/stack.

Scope cut: private by construction. The detail is parsed protocol fields only, with no free text from either stream. Stdout lines are never copied. A message that looks like snapshot content, or was cut at the line limit, is withheld whole and named in withheldFields. From stderr, only a strict startup-failure envelope is read.

Refs #4005 (diagnostics, not the fix).

Validation

  • Tests first; each regression failed on the prior head.
  • pnpm check:affected --run --head 5e9942f87c29ce45fa0b88595c08f0a75164a9fc passed, plus provider-integration, eager-closure and scoped coverage (100%).
  • Device (emulator-5554): a force-stopped helper reports shortMsg="Process crashed." on the session-exit, fallback and one-shot paths, with no payloadBase64, <node or text=" in errors or logs. See the comment.

🤖 Generated with Claude Code

thymikee and others added 2 commits October 28, 2026 12:47
…per exits without output

Refs #4005

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
agent-device-website Ignored Ignored Preview Oct 28, 2026 4:57pm UTC

Request Review

@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 6 files

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

View guided diff | Turn on auto-fix | Re-trigger cubic

@github-actions

github-actions Bot commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.56 MB 5.56 MB +4.1 kB
Package (unpacked) 5.56 MB 5.56 MB +4.1 kB
Package (download) 1.69 MB 1.69 MB +1.4 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 40.9 ms 40.5 ms -0.4 ms
CLI --help 82.6 ms 81.6 ms -1.0 ms

…s and keep the real cause

Strip encoded payloads and raw XML before the output tail is built, keep the last 400
characters after redaction, read multiline and startup-failure records, record exits after
the session was ready, and carry the host signal through the executor result.

Refs #4005

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 11 files (changes from recent commits).

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

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/host-kit/src/internal/exec.ts Resolved
…ad exits after the streams close

Treat XML attribute syntax as snapshot content even without its opening tag, read the session
exit result after the child's streams close, cap single-line shortMsg/longMsg, accept only
safe-integer result codes, and report the signal from every ExecResult producer.

Refs #4005

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 5 files (changes from recent commits).

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

View guided diff | Turn on auto-fix | Re-trigger cubic

thymikee and others added 2 commits October 28, 2026 14:19
…nd keep long diagnostic lines

Fold the exit-result code into instrumentation-helper.ts so the eager closure does not grow.

Refs #4005

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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 6 files (changes from recent commits).

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

View guided diff | Turn on auto-fix | Re-trigger cubic

@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 7 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 | Turn on auto-fix | Re-trigger cubic

Comment thread packages/host-kit/src/internal/exec.ts Resolved
@thymikee

Copy link
Copy Markdown
Member Author

I reviewed 9eddef3 by reading the code. I did not run tests, and I had no emulator. The change does not look ready yet, for two reasons. The failure path gets slower, and there is no live device evidence.

In snapshot-helper-session.ts:79, any failed session request whose helper is still alive now waits the full SESSION_EXIT_EVIDENCE_GRACE_MS (250 ms). This covers a socket timeout, truncated XML as in #4005 run B, and a malformed response. The helper does not exit in these cases, so the wait ends only when retireFailedAndroidSnapshotHelperSession kills it. The wait also sits inside failedAfterMs, so it lengthens the backoff for short failures. Every session-to-one-shot fallback becomes 250 ms slower, which conflicts with "no fallback change". It hits the route #4005 reports, a fallback within about 1 s of a transition. The rule should be: collecting exit evidence must not add latency to a session the host is about to kill. Can recordInstrumentationProcess emit android_snapshot_helper_session_exited from its own close handler when the host did not initiate the stop? A flag set by stopAndroidSnapshotHelperSession before it kills would mark that. Then the awaited wait in captureFromAndroidSnapshotHelperSession and the drain in stopAndroidSnapshotHelperSession can go.

The PR changes the live session lifecycle: new stream listeners on the session child, an awaited drain in stop, and a wait before retire (snapshot-helper-session-lifecycle.ts:474). It ships only hand-written AOSP fixtures, and the body says no device run was done. Please run on an Android emulator with a persistent session open. First, kill the helper mid-session with adb shell am force-stop com.callstack.agentdevice.snapshothelper, then run snapshot. The request log must show android_snapshot_helper_session_exited with instrumentation.exitCode/resultCode or shortMsg, followed by a successful one-shot snapshot. Second, force a one-shot failure, for example by killing the helper during capture. The client error's details.instrumentation must hold real device output, with no XML or payloadBase64 in outputTail. Please paste both excerpts.

The cubic-dev-ai P1 thread on one shared line buffer for stdout and stderr still stands: #4021 (comment). One P3 thread on a dropped oversized line also still applies. The other P3 thread, on the wording of the signal comment in host-kit/src/internal/exec.ts, is a wording nit and still applies. Two P1 threads do not apply, so please resolve them. #4021 (comment): the helper sends chunked XML as base64 in payloadBase64, and inline XML starts with <?xml or <hierarchy, which the filter strips, so a record cannot begin with a bare attribute. #4021 (comment): the scan cut is 16,384 chars from the end, but outputTail keeps only the last 400 chars, so a key split at the cut never reaches the tail.

Production code grows by about 336 lines across host-kit, kernel and platform-android. Could the change be smaller? Keep readInstrumentationExitResult for the one-shot and exited-before-ready errors. That is the issue's ask and covers #4005 runs A to C. For a session that dies after ready, let the process record emit the exit event from its close listener. That removes the 250 ms wait, the drain in stop, and SESSION_EXIT_EVIDENCE_GRACE_MS. It also gives one line buffer per stream, which closes the shared-buffer thread.

Not blocking, and fine to take or leave: SESSION_EXIT_EVIDENCE_GRACE_MS duplicates ANDROID_SNAPSHOT_HELPER_HOST_PROCESS_EXIT_GRACE_MS (250 in snapshot-helper-retirement.ts), which goes away if the emit moves into the close handler. MESSAGE_FIELD_MAX_CHARS = 2048 in instrumentation-helper.ts is never observable, because redaction truncates non-stderr strings to 400 chars. Cap those fields at REDACTED_STRING_MAX_LENGTH or leave the boundary to own the bound. The PR body is out of date: it says 6 files and "no fallback change", and it does not mention the new android_snapshot_helper_session_exited phase, the host-kit ExecResult.signal field, or the kernel redaction exports.

CI is still pending. All 13 remaining checks are queued or in progress on 9eddef3, and none has failed. Lint/Typecheck, Coverage and the Android smoke route overlap this diff, so their results matter. Before merge, remove the 250 ms wait from the session failure path, split the shared stdout/stderr buffer, and attach the live emulator run above.

thymikee and others added 3 commits October 28, 2026 14:40
… filtered stderr tail

Stdout is no longer copied: the exit result holds the parsed protocol fields and a bounded
stderr tail with one parser state per stream. Drops the stdout tail filter.

Refs #4005

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ne reader

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 6 files (changes from recent commits).

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

View guided diff | Turn on auto-fix | Re-trigger cubic

…g a cut unbroken stderr line

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 4 files (changes from recent commits).

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

View guided diff | Turn on auto-fix | Re-trigger cubic

thymikee and others added 3 commits October 28, 2026 15:01
…etails

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…d snapshot without finalizing

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e stderr tail

Read startup failures from strict whole-line envelopes, sanitize executor timeouts at the
capture boundary, bound the exit drain, and stop delaying live-helper retirement.

Refs #4005

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 10 files (changes from recent commits).

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

View guided diff | Turn on auto-fix | Re-trigger cubic

…fore, strip only stdout

Restore the snapshot helper failure reason and the stderr detail, strip stdout from executor
timeouts, share one signal result builder in the foreground executors, and make the
live-session retirement test independent of wall-clock time.

Refs #4005

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 8 files (changes from recent commits).

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

View guided diff | Turn on auto-fix | Re-trigger cubic

thymikee and others added 2 commits October 28, 2026 15:58
…keep helper-bearing errors as on main

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Drop the attribute list: a message value, or a continuation line, that holds an
attribute-like assignment, markup, an entity, a payload record or a cut line is withheld
whole and named in withheldFields. The abort message is checked like the others.

Refs #4005

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 | Turn on auto-fix | Re-trigger cubic

…o one line

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

Copy link
Copy Markdown
Member Author

Thanks for the update. At c97885f the entity matching, the ABORTED record handling and the withheldFields doc are fixed, but the change still has no run on a real device.

The PR changes the live persistent-session lifecycle: new stdout/stderr listeners on the session child, a closed-or-250ms drain before the android_snapshot_helper_session_exited diagnostic (snapshot-helper-session-lifecycle.ts#L553), a deferred exited-before-ready reject, and an instrumentation detail on one-shot failure errors. The only proof is hand-written AOSP fixtures, and no comment shows that a real am instrument -w force-stop or crash produces the parsed fields, or that close fires within the drain on a real adb. So the #4005 diagnostics goal is unproven on the device it targets. Please run an Android emulator with a persistent session open. First, run adb shell am force-stop com.callstack.agentdevice.snapshothelper mid-session, then run snapshot. Paste the request-log android_snapshot_helper_session_exited entry (or the android_snapshot_helper_session_fallback entry) with its instrumentation object (exitCode plus resultCode/shortMsg/aborted), followed by the successful one-shot snapshot. Second, force a one-shot helper failure, for example by killing the helper during a one-shot capture. Paste the client error's details.instrumentation and show that no XML and no payloadBase64 text appears in it.

Not blocking, take or leave: in instrumentation-helper.ts#L163 the bare &# alternative in MARKUP_RE already matches every numeric entity, so the #[0-9]+; and #x[0-9A-Fa-f]+; branches are dead, and /[<>]|&[A-Za-z][A-Za-z0-9]*;|&#|payloadBase64/i says the same thing.

I did not run the tests or any device. I judged the new tests by reading the parser.

None of the earlier inline threads still apply, so you can resolve them: the stderrTail thread (stderr stays as on main by maintainer decision, tracked in #4026) #4021 (comment); the dropped truncation flag (no flag exists, and the assembler only reads FAILED envelopes) #4021 (comment); raw output on ready-timeout and exited-before-ready errors (they now carry instrumentation) #4021 (comment); early rethrow of parse failures (only reports with details.helper rethrow, and instrumentation is added) #4021 (comment); assemblers ending on a non-final result (only exitResult() after close finalizes) #4021 (comment); the 200 ms wall-clock assertion (gone, fake timers now) #4021 (comment); the failure-reason selector claim (reason composed as on main, no new selector) #4021 (comment); two stderr exposure threads (unchanged from main, tracked in #4026) #4021 (comment) and #4021 (comment); MARKUP_RE entities (now matches any &name;) #4021 (comment); the ABORTED record (now returns undefined, so open closes) #4021 (comment); the withheldFields doc (now names both cases) #4021 (comment).

Smoke Tests is still queued, not failed. Its Android route runs snapshot through the helper capture and session paths this diff changes, so its result matters. Before merge, attach the two emulator excerpts and let Smoke Tests finish green.

…kup rule

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

Copy link
Copy Markdown
Member Author

Device run on emulator-5554 (sdk_gphone64_arm64, API 35), daemon built from this branch (helper 0.21.34-dev, packaged locally), Settings open, --debug request logs.

1. Persistent session killed (am force-stop com.callstack.agentdevice.snapshothelper)

Idle kill, then snapshot -i (the exit had happened about 4 s earlier). The next acquire found the ended session:

{"phase":"android_snapshot_helper_session_exited","level":"warn","data":{"deviceKey":"android:emulator-5554","capturedCount":1,"instrumentation":{"exitCode":0,"resultCode":0,"statusCode":2,"shortMsg":"Process crashed."}}}

It then logged android_snapshot_helper_session_ready for a new session, and the snapshot succeeded (Snapshot: 16 visible nodes (30 total)).

Kill 150 ms into a snapshot (request in flight):

{"phase":"android_snapshot_helper_session_fallback","level":"warn","data":{"reason":"Android snapshot helper session returned malformed output","instrumentation":{"exitCode":null,"resultCode":0,"statusCode":2,"shortMsg":"Process crashed."},"uiAutomationConnectionTimeout":false}}
{"phase":"android_snapshot_helper_capture","durationMs":370,"data":{"transport":"instrumentation"}}

The one-shot capture then succeeded. Here the host process had not reported its exit yet (exitCode: null) while the instrumentation had already printed shortMsg/resultCode. Retirement did not wait for it.

Finding: I did not instrument the close event itself. In the idle case the process had been gone for seconds, and in the in-flight case retirement is deliberately immediate, so I never saw an exit-then-close gap on real adb and the 250 ms drain was not exercised.

2. One-shot failure (AGENT_DEVICE_ANDROID_SNAPSHOT_HELPER_SESSION=0, helper killed 300 to 500 ms into the capture; 3 of 6 attempts failed)

Error (COMMAND_FAILED): Android snapshot helper failed: Android snapshot helper output could not be parsed
{
  "stderr": "",
  "exitCode": 0,
  "instrumentation": { "exitCode": 0, "resultCode": 0, "shortMsg": "Process crashed." },
  "androidSnapshotHelperFailureReason": "Android snapshot helper output could not be parsed",
  "dispatched": "no"
}

The request-log entry carries the same object:

{"phase":"android_snapshot_helper_failed","level":"error","data":{"reason":"Android snapshot helper output could not be parsed","instrumentation":{"exitCode":0,"resultCode":0,"shortMsg":"Process crashed."}}}

grep -c over every client error output and every request log of the run: payloadBase64 0, <node 0, text=" 0.

Real am instrument output for a force-stopped helper is shortMsg=Process crashed. plus INSTRUMENTATION_CODE: 0 with exit status 0, which the hand-written fixtures did not cover. Session closed, daemon stopped, emulator left running.

The optional MARKUP_RE simplification is in 5e9942f87c, and the gate passed on it. All 29 inline threads are resolved, including the 13 you listed.

@thymikee

Copy link
Copy Markdown
Member Author

The review of 5e9942f finds no code problems. Since the earlier review at c97885f, the evidence gap is closed: the author's pasted logs from two real-device runs show the diagnostic fields on a real force-stop, in both the idle and the in-flight path.

All 18 checks pass at this commit, and there are no conflicts. The last change is a regex simplification that does not change the matched language, so it cannot change any job result. Nothing else blocks merge.

I did not run tests or a device myself. Two limits stay, and neither blocks merge. The closed-or-250 ms drain was not seen on real adb, because no exit-then-close gap appeared, so only the fake-timer tests cover it. The one-shot failure showed exitCode 0 with no crash stack, because a force-stop prints only shortMsg and INSTRUMENTATION_CODE: 0, so real crash-stack and abort shapes are covered only by fixtures.

On the earlier threads, all already resolved: the three about the markup regex (#4021 (comment)), the one-line abort message (#4021 (comment)), and the withheldFields doc (#4021 (comment)) are fixed at this commit. The drain-test thread (#4021 (comment)) is fixed too: the test uses fake timers, and the remaining Date.now check is a loose 2 s bound. The three stderr threads (#4021 (comment), #4021 (comment), #4021 (comment)) do not apply: stderr handling is unchanged from main and is tracked in #4026.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 28, 2026
@thymikee
thymikee merged commit c8772b8 into main Oct 29, 2026
18 checks passed
@thymikee
thymikee deleted the maint/4005-android-helper-exit-diagnostics branch October 29, 2026 09:03
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.

1 participant