Skip to content

test(hermes): give quit-time Hermes loads their own complete process census - #14795

Merged
teamleaderleo merged 1 commit into
mainfrom
fix/hermes-quit-save-flake
Sep 26, 2026
Merged

teamleaderleo merged 1 commit into
mainfrom
fix/hermes-quit-save-flake

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

HermesFirstClassSupportTests has been failing in "app-host unit tests (changed suites)" on unrelated PRs (#14783 twice, #14788):

  • quitTimeSaveRevalidatesCachedHermesProcess at HermesFirstClassSupportTests.swift:939
  • freshSynchronousLifecycleLoadDiscoversNewHermesSession at :1022

Both failures print RestorableAgentSessionIndex(entriesByPanel: [:], ..., isComplete: false, ...), which is exactly RestorableAgentSessionIndex.unavailable: ProcessDetectedResumeIndexes.loadOnWorker returns it when the process census is unavailable or incomplete.

Root cause: loadOnWorker read the whole host's process table through the app's shared ProcessSnapshotService. That census fails closed by design (DarwinProcessEnumerator and CmuxTopProcessSampler.enrich count any PID that exits or is reused between listing and reading as missing, and enumerationIsComplete goes false). On a busy CI mini with neighbouring compile jobs spawning and reaping processes, that is routine, so the Hermes assertions were really measuring host process churn.

Change

  • ProcessDetectedResumeIndexes.loadOnWorker / loadFreshOnWorker take an optional processSnapshotService (nil keeps today's shared app census; no production call site changes).
  • The two Hermes tests inject a complete, empty census (a private CmuxTopProcessReading that enumerates nothing). The fixture PIDs are never live, so the tests still exercise the real revalidation / fresh-discovery logic, without reading the host process table.

Other suites seen red today (different causes, not in this PR)

  • WorkspaceForkConversationContextMenuTests (Prevent deep process-tree stack overflow #14785, also main run 36197594615): the four sharedForkProbe* executable-watch tests. The loader is injected there; the likely gate is the file-descriptor headroom check in SharedLiveAgentIndex+ScheduledHibernation.swift (soft RLIMIT_NOFILE minus open fds minus a 128 reserve), which depends on how many fds earlier suites left open in the shared test host.
  • DeadKeyCompositionRegressionTests.testOptionDeadKeyUsesGhosttyTranslationInsteadOfStartingComposition (Keep unrelated file descriptors out of notification hooks #14789): Option-as-Alt side claim; config / keyboard state, unrelated to the process census.
  • CloudTunnelLaunchGateTests: not in the failing logs inspected here.

Test plan

  • CI app-host changed suites: HermesFirstClassSupportTests green

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes two flaky HermesFirstClassSupportTests that read the whole host process table through the app's shared census, which fails closed whenever any process on the machine exits mid-scan — routine on busy CI runners.

  • Adds an optional processSnapshotService to loadOnWorker/loadFreshOnWorker; nil keeps the app's shared census and no production call sites change.
  • The two Hermes lifecycle tests now inject a complete, empty census so they exercise real revalidation / fresh-discovery logic without depending on live host processes.

Written for commit fdea334. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Tests
    • Updated process-index loading tests to use controlled snapshot data when checking cached and fresh results.
  • Internal Improvements
    • Process-index loading now supports a supplied snapshot provider while retaining the existing default behavior when none is provided. No user-facing changes are noted.

…census

ProcessDetectedResumeIndexes.loadOnWorker read the whole host's process
table through the app's shared ProcessSnapshotService. That census fails
closed (enumerationIsComplete = false) whenever any process on the machine
exits between listing and reading it. On a busy CI mini that is routine,
so the two Hermes lifecycle tests got RestorableAgentSessionIndex.unavailable
and failed at HermesFirstClassSupportTests.swift:939 / :1022 on unrelated PRs.

loadOnWorker and loadFreshOnWorker now take an optional
processSnapshotService (nil keeps the app's shared census), and the tests
inject a complete, empty census so they exercise Hermes revalidation
instead of host process churn.

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

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 10da075d-ead3-435f-a1a1-3b84c2982b20

📥 Commits

Reviewing files that changed from the base of the PR and between c07c13a and fdea334.

📒 Files selected for processing (2)
  • Sources/ProcessDetectedResumeIndexes.swift
  • cmuxTests/HermesFirstClassSupportTests.swift

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


📝 Walkthrough

Walkthrough

Process-index loaders now accept an optional process snapshot service and pass it to cached or fresh snapshot capture. Tests use a fixture-backed service with an empty process census.

Changes

Process Snapshot Injection

Layer / File(s) Summary
Pass the service through index loading
Sources/ProcessDetectedResumeIndexes.swift
The worker loaders accept an optional snapshot service. Loading uses cached capture when a maximum snapshot age is set, and fresh capture otherwise.
Test loading with a fixture service
cmuxTests/HermesFirstClassSupportTests.swift
Process-index tests pass a fixture-backed service. Its reader reports a complete, empty process listing and no process details.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: austinywang

Merge Risk: ⚪ Minimal · up to fdea3

The fixture-backed tests can use an empty process census without changing the app’s default census behavior. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 23 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description provides a strong problem statement and implementation summary, but it does not include the required Testing section with executed commands and results. The checklist is also missing, … Add a Testing section that names the test command or CI lane, states what passed, and identifies anything not verified. Add the required Checklist section and mark each applicable item. Include a Demo Video section only if this change is co…
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (23 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the Hermes test change and the use of a complete process census. It does not mention the fresh-load test or optional API change, but it remains specific and related to the…
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.
Cmux Cloud Persistent Session And Early Input ✅ Passed PASS: The pull request changes only process snapshot loading and Hermes tests. It does not change Cloud terminal creation, cmux-tui transport, manual renderer admission, input routing, authentication,…
Cmux Swift Actor Isolation ✅ Passed PASS: The production diff only injects ProcessSnapshotService into existing worker methods and forwards it to snapshot capture. ProcessSnapshotService is an actor, and the captured types and worke…
Cmux Swift Blocking Runtime ✅ Passed The production diff only adds an optional ProcessSnapshotService parameter and forwards it to existing snapshot capture APIs. The added lines introduce no semaphore, blocking wait, sleep, delayed disp…
Cmux Browser Automation Off-Main ✅ Passed PASS. The PR changes only Sources/ProcessDetectedResumeIndexes.swift and cmuxTests/HermesFirstClassSupportTests.swift. It adds no browser.* socket commands and does not modify `TerminalControlle…
Cmux Expensive Synchronous Load ✅ Passed The production diff only injects an optional process snapshot service into the existing worker loader. It does not add or move an expensive agent-history load. RestorableAgentSessionIndex.load(...) …
Cmux Cache Substitution Correctness ✅ Passed The diff does not replace a fresh authoritative read with a cache. It adds an optional ProcessSnapshotService injection to ProcessDetectedResumeIndexes; nil preserves the existing app service an…
Cmux No Hacky Sleeps ✅ Passed PASS. The PR changes only Swift files. The configured check covers TypeScript, JavaScript, shell, and non-Swift build/runtime scripts. The diff adds no fixed sleeps, timers, polling, or wall-clock wai…
Cmux Algorithmic Complexity ✅ Passed The production diff only adds an optional ProcessSnapshotService parameter and forwards it to existing snapshot capture calls. It introduces no loops, scans, sorting, filtering, joins, or per-target…
Cmux Swift Concurrency ✅ Passed The diff adds an optional process snapshot service and forwards it through existing async/await capture calls. It adds no Dispatch queues or groups, Combine state, completion-handler API, or fire-and-…
Cmux Swift @Concurrent ✅ Passed The changed worker path keeps ProcessDetectedResumeIndexes.loadOnWorker annotated with @concurrent (with @Sendable for older compilers). The added processSnapshotService parameter only injects…
Cmux Swift Package Boundaries ✅ Passed PASS. The production diff only adds an optional ProcessSnapshotService injection point and forwards it to existing snapshot capture paths. It does not introduce or materially expand independent doma…
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only Sources/ProcessDetectedResumeIndexes.swift and cmuxTests/HermesFirstClassSupportTests.swift. It does not change a Package.swift, Package.resolved, .gitignore, workflow, o…
Cmux Swift Logging ✅ Passed The production diff only adds an optional process snapshot service and passes it to snapshot capture. It adds no print, debugPrint, dump, NSLog, Logger, file logging, or diagnostic output. T…
Cmux User-Facing Error Privacy ✅ Passed PASS. The production diff only adds an optional process snapshot service and forwards it to existing capture calls. It adds no user-facing errors, alerts, command output, API bodies, or recovery copy.…
Cmux Full Internationalization ✅ Passed The diff changes process-snapshot injection in production code and adds a test-only process census fixture. It adds no user-facing Swift text, localization keys, catalogs, web messages, or locale data…
Cmux Swiftui State Layout ✅ Passed PASS. The PR changes process-snapshot loading and Hermes test fixtures only. The changed files contain no SwiftUI views or new ObservableObject, @Published, GeometryReader, lazy/list row store referen…
Cmux Architecture Rethink ✅ Passed The diff does not introduce any prohibited timing, blocking, polling, lock, observer, side-channel, duplicate wiring, or UI lifecycle ownership pattern. It adds an optional process snapshot dependency…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The pull request changes only process snapshot loading and Hermes test fixtures. The authoritative diff contains no NSWindow, NSPanel, NSWindowController, SwiftUI Window, or WindowGroup changes, and i…
Cmux Source Artifacts ✅ Passed The PR changes only two existing Swift source files: Sources/ProcessDetectedResumeIndexes.swift and cmuxTests/HermesFirstClassSupportTests.swift. The additions are production source changes and a …
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The production diff in Sources/ProcessDetectedResumeIndexes.swift adds optional process-service dependency injection to existing worker methods. It adds no #if DEBUG or test-build guard, no …
Full details: Description check

Explanation

The description provides a strong problem statement and implementation summary, but it does not include the required Testing section with executed commands and results. The checklist is also missing, and the test plan remains unchecked.

Resolution

Add a Testing section that names the test command or CI lane, states what passed, and identifies anything not verified. Add the required Checklist section and mark each applicable item. Include a Demo Video section only if this change is considered user-facing behavior.

  • Fix all pre-merge checks with AI
✨ 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

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.

@teamleaderleo
teamleaderleo merged commit e0e635e into main Sep 26, 2026
63 of 64 checks passed
@teamleaderleo
teamleaderleo deleted the fix/hermes-quit-save-flake branch September 26, 2026 04:41
@github-actions

Copy link
Copy Markdown
Contributor

Merge receipt for fdea334a92: every check was green at merge (16 verified; 14 skipped by policy). Full suite runs on main after merge.

rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 26, 2026
b92d99c ci: seed far main pushes in their own lane, hold the seed root, prefetch between trusted seeds (manaflow-ai#14792)
6f3af0a docs(agents): catch-up is automatic; merge main by hand only when needed (manaflow-ai#14798)
e0e635e test(hermes): give quit-time Hermes loads their own complete process census (manaflow-ai#14795)
d09a5e4 ci: build main into an idle owned root's kept state; keep seeds down to 150 GiB free (manaflow-ai#14793)
bf8b326 ci(e2e): UI runs adopt the app and UI test bundle PR CI compiled (manaflow-ai#14776)
c07c13a Notify iroh of iOS network changes so dead direct paths drop immediately (manaflow-ai#14720)
46444ea Discard phantom (0-tab) windows on session restore to stop WindowServer wedge (manaflow-ai#14788)

# Conflicts:
#	.github/workflows/ci-guards.yml
#	.github/workflows/seed-derived-data.yml
#	.github/workflows/test-e2e.yml
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