Skip to content

fix(shared): preserve recorder artifacts during signal shutdown - #3049

Merged
quanru merged 5 commits into
mainfrom
fix/recorder-observation-finalization
Aug 28, 2026
Merged

fix(shared): preserve recorder artifacts during signal shutdown#3049
quanru merged 5 commits into
mainfrom
fix/recorder-observation-finalization

Conversation

@quanru

@quanru quanru commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • capture terminal Ctrl+C as raw input so package runners remain alive while the recorder finalizes observation artifacts
  • keep SIGINT, SIGTERM, and SIGHUP handlers active through observation stop, export, manifest write, and cleanup
  • restore the previous terminal raw/flowing state on success and setup failure
  • let a second explicit Ctrl+C restore the terminal and force exit with code 130 if finalization hangs
  • defer Linux headless Xvfb cleanup until the recorder CLI process has fully exited
  • cover signal forwarding, terminal lifecycle, manifest persistence, cleanup ordering, Xvfb lifetime, and CLI option propagation with focused tests

Root cause

In a foreground PTY, Ctrl+C targets the entire process group. A package runner such as pnpm exits at the same time as the Midscene child, forwards SIGTERM, and can trigger SIGHUP when the controlling PTY closes. The recorder had already started asynchronous observer.stop(), but the process was terminated before the manifest and final frames directory were persisted.

Keeping signal listeners through finalization protects externally delivered termination. Capturing Ctrl+C as terminal input additionally prevents the package runner from exiting in the first place, so it waits for Midscene to save and return normally.

The temporary Linux acceptance test exposed a second shutdown edge case: libnut keeps a process-wide X11 connection open. Stopping the CLI-owned Xvfb from destroy() or a Node exit handler triggers an XIO fatal error even after the manifest is saved. The CLI now delegates Xvfb shutdown to a detached monitor that waits for the owner process to disappear. SDK/API-owned Xvfb instances still stop synchronously during normal destroy().

Follow-up safeguards

The finalization guard distinguishes automatic follow-up signals from an explicit second terminal Ctrl+C. SIGTERM and SIGHUP remain absorbed while saving, but pressing Ctrl+C again restores stdin and exits with the conventional SIGINT code 130. In Linux headless mode, the detached monitor keeps the display alive through representative-frame capture and CLI teardown, then removes it after process exit.

Validation

  • pnpm run lint
  • pnpm exec nx test @midscene/shared (500 tests passed)
  • pnpm exec nx test @midscene/computer (96 tests passed)
  • second-Ctrl+C child-process probe (exit code 130)
  • pnpm exec nx run-many -t build -p @midscene/shared @midscene/computer @midscene/android
  • real-device recording on Samsung SM-N9860 (R5CN90V1AYP) through a pnpm package-runner PTY; Ctrl+C exited with code 0 and saved a v1 observation manifest containing 14 frames and 10 screenshots
  • midscene-android assert --record <manifest> --prompt "The observation shows the Samsung Android device transitioning from the home screen to the Settings screen." (Assertion passed.)
  • temporary GitHub Actions acceptance: PR #3060 / successful job
    • launched the real midscene-computer record start CLI through pnpm in a PTY on Ubuntu/Xvfb
    • changed the virtual desktop from dark to blue, sent raw Ctrl+C, and observed recording_stopping, Observation record saved, command_done: ok, and exit code 0
    • validated a v1 manifest with 11 logical frames and 3 JPEG files, no leftover temporary frame directory, and removal of the Xvfb lock
    • ran the real midscene-computer assert --record command against that manifest (Assertion passed.)
  • thermo-nuclear maintainability review: no structural blockers, no new 1k-line threshold crossing, and terminal lifecycle logic isolated behind one disposable guard

Prepatch

  • npm beta: 1.12.2-beta-20260827111913.0 (@midscene/core, @midscene/android, and @midscene/computer)
  • release workflow: https://github.com/web-infra-dev/midscene/actions/runs/33066736758 (npm release and Studio packaging succeeded across macOS ARM, macOS Intel, Linux, and Windows)
  • note: this beta predates the detached-monitor Xvfb follow-up commits added after the temporary Computer CI acceptance

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9cfbb971d3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/shared/src/cli/interrupt.ts
@quanru
quanru marked this pull request as draft August 27, 2026 09:17
@quanru

quanru commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator Author

Temporary real Computer recorder acceptance passed on GitHub Actions: https://github.com/web-infra-dev/midscene/actions/runs/33138403058/job/98743638436

The job launched the actual midscene-computer record start CLI through pnpm in a PTY on Ubuntu/Xvfb, changed the virtual desktop, sent raw Ctrl+C, observed a normal exit after Observation record saved, validated the v1 manifest (11 logical frames / 3 JPEG files), confirmed Xvfb cleanup, and completed a real midscene-computer assert --record with Assertion passed.

This run also exposed the XIO-on-Xvfb-shutdown edge case; commits 2b367ef03 and 70150efbf fix it by deferring CLI-owned Xvfb cleanup to a detached post-exit monitor while preserving synchronous SDK/API cleanup.

@quanru
quanru marked this pull request as ready for review August 28, 2026 07:34
@quanru
quanru merged commit 87b0221 into main Aug 28, 2026
17 checks passed
@quanru
quanru deleted the fix/recorder-observation-finalization branch August 28, 2026 07:55
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