Skip to content

fix: Agents now see the Unity main-thread stall notice even when stderr is not a terminal - #3416

Merged
hatayama merged 1 commit into
mainfrom
fix/main-thread-stall-notice-without-tty
Oct 29, 2026
Merged

hatayama merged 1 commit into
mainfrom
fix/main-thread-stall-notice-without-tty

Conversation

@hatayama

@hatayama hatayama commented Oct 29, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • When the Unity Editor main thread has been stuck for 30 seconds, the CLI now tells you on stderr even when stderr is not a terminal, so agents see why a command is waiting.

Refs #3411. The Unity-side fail-fast for a command still waiting to start (MAIN_THREAD_BLOCKED) comes in a separate PR.

User Impact

  • Before: the "Unity main thread stuck 30s; check modal/long operation..." notice was only drawn on the spinner, and the spinner is shown only when stderr is a terminal. Agents read stderr through a pipe, so a command waiting on a modal dialog printed nothing until it finished or the CLI gave up after 5 minutes.
  • After: without a spinner the same notice is written to stderr as a plain line, at most one line per minute so a long stall does not flood the output. With a spinner nothing changes: the notice still replaces the spinner text in place.

Changes

  • The progress handler now passes stall notices to a new spinner notice method that falls back to a rate-limited plain line when the spinner is disabled. A disabled spinner also records that it was stopped, so a notice arriving after the command ends writes nothing.
  • Shared release input stamps are refreshed for the cli/common change.

Verification

  • go test ./ui/... ./unityipc/... ./clicore/... in cli/common, including five new spinner tests (plain line, one line per minute, nothing after stop, in-place update with a spinner, progress handler writing the notice without a spinner).
  • gofmt -l, go vet ./..., and golangci-lint run ./... are clean in all four Go modules; go test ./... passes except tests that bind Unix sockets or create directories under /tmp, which the local sandbox denies (left to CI).

View guided diff

Agents read stderr through a pipe, so the spinner is disabled and the
30-second stall notice from the heartbeat was dropped: a command waiting
on a blocked Editor main thread looked silent until it finished or the
CLI gave up. Notices now fall back to a plain stderr line, at most one
per minute so a long stall does not pile up lines; an enabled spinner
still shows them in place.
@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 4ecde68 and 7716452.

Files selected for processing: 4
  • cli/common/ui/spinner.go
  • cli/common/ui/spinner_test.go
  • cli/dispatcher/shared-inputs-stamp.json
  • cli/project-runner/shared-inputs-stamp.json

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

Walkthrough

Progress messages now use spinner notices. Disabled spinners write notices as plain lines, limited to one per 60 seconds, and ignore notices after Stop. Enabled spinners display notices in place.

Changes: Spinner progress notices

Layer / File(s) Summary
Notice behavior
cli/common/ui/spinner.go, cli/dispatcher/shared-inputs-stamp.json, cli/project-runner/shared-inputs-stamp.json
TerminalSpinner adds an injectable clock and notice timestamp. The progress callback sends non-connection messages to Notice. Disabled spinners write notices as lines unless stopped or rate-limited.
Notice behavior tests
cli/common/ui/spinner_test.go
Tests cover plain-line output, the 60-second interval, notices after Stop, in-place display for enabled spinners, and progress messages without a spinner.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related issues

  • Issue 3411 — The change surfaces stall notices as lines when the spinner is disabled, addressing the issue’s silent-wait behavior. The issue’s fail-fast behavior remains outside this change.
Merge Risk: Minimal · up to 77164

Stall notices are now available on piped stderr. No actionable merge-blocking risk was identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: showing the Unity main-thread stall notice when stderr is not a terminal.
Description check Passed The description directly explains the stderr fallback, rate limiting, spinner behavior, tests, and verification results for the changeset.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 6 functions across 2 files. (2 skipped: 2 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@hatayama
hatayama merged commit eba0ea4 into main Oct 29, 2026
16 checks passed
@hatayama
hatayama deleted the fix/main-thread-stall-notice-without-tty branch October 29, 2026 07:03
tongguang added a commit to tongguang/unity-cli-loop that referenced this pull request Oct 29, 2026
Integrate upstream hatayama#3412 (4ecde68) and hatayama#3416 (eba0ea4).
Keep the Tuanjie-only tloop command, owned size name, skill/help text,
and SessionState prefix. Regenerate skill copies, the tool catalog,
and shared-input stamps.

Validation:
- scripts/check-go-cli.sh: all four Go modules and cross-platform builds.
- scripts/check-tuanjie.ps1: zero compile errors or warnings; 26 affected EditMode tests passed.
- Live Tuanjie checks: temporary size creation/removal and selection restoration; Simulator rejects size changes and reset.
- Skill size, asmdef reference policy, file length, and C# complexity checks passed.
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.

1 participant