Skip to content

fix(desktop): measure adaptive stream flush through the deferred commit frame - #72801

Closed
Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/desktop-stream-flush-frame-cost
Closed

fix(desktop): measure adaptive stream flush through the deferred commit frame#72801
Adolanium wants to merge 1 commit into
NousResearch:mainfrom
Adolanium:fix/desktop-stream-flush-frame-cost

Conversation

@Adolanium

Copy link
Copy Markdown
Contributor

What does this PR do?

The adaptive stream-flush floor from #72504 never engaged on the streaming path. runFlush timed only flushQueuedDeltas(), the synchronous store write. While a session streams, the $messages publish (React commit + Streamdown re-parse) is deferred to a view-sync rAF in syncSessionStateToView, so it lands in a later frame, after the measurement. The measured cost stayed near zero, the adaptive branch needs more than ~11ms to engage, and the floor collapsed to the fixed 33ms gap at any load. Multi-stream streaming could still starve the main thread of idle frames and hitch typing, the failure mode the floor was built to prevent.

runFlush now keeps the write cost as a fallback and extends the measurement through a rAF registered after the view-sync one. It runs in the same frame right after the deferred commit, and the rAF timestamp marks frame start so only in-frame work is counted, not the vsync wait. A stale callback from before a newer flush is ignored. A hidden renderer that never fires rAF keeps the write-cost fallback, so delivery in parked renderers is unchanged (still timer-driven, never frame-gated).

Related Issue

Fixes #72799

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Security fix
  • Documentation update
  • Tests (adding or improving test coverage)
  • Refactor (no behavior change)
  • New skill (bundled or hub)

Changes Made

  • apps/desktop/src/app/session/hooks/use-message-stream/index.ts: measure flush cost through the deferred view-sync frame (write cost + in-frame work via the rAF timestamp), keep the write-cost fallback for hidden renderers, ignore a stale measurement once a newer flush has started.
  • apps/desktop/src/app/session/hooks/use-message-stream/delta-flush.test.tsx: cover the frame-measured adaptive floor, the no-frame fallback, and the stale-callback guard.
  • apps/desktop/src/app/session/hooks/use-message-stream/stream-flush.test.tsx: update the parked-frame test. The flush still never waits on a frame, but runFlush now registers a measurement callback that is allowed to wait.

How to Test

  1. npx vitest run --project ui src/app/session/hooks/use-message-stream/
    • 14 files, 59 tests passed (3 new in delta-flush.test.tsx)
  2. Proof of the bug on main: with index.ts reverted, the new tests "stretches the flush gap when the deferred commit frame is expensive" and "ignores a late frame measurement once a newer flush has started" fail because no frame measurement exists. With this branch, all pass.
  3. Wider net: npx vitest run --project ui src/app/session/hooks/
    • 30 files, 352 tests passed. npm run typecheck clean. npx eslint on the touched files clean (one pre-existing no-restricted-globals warning in delta-flush.test.tsx).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the vitest suites above and they pass. Full desktop e2e not run here. Change is isolated to the stream flush scheduler + tests.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) - or N/A (code comments on the measurement path only)
  • I've updated cli-config.yaml.example if I added/changed config keys - or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows - or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide - rAF + timer only, no platform API. Reproduced and tested on Windows.
  • I've updated tool descriptions/schemas if I changed tool behavior - or N/A

Screenshots / Logs

# On main (index.ts reverted): the new tests prove no frame measurement exists
 Tests  2 failed | 3 passed (5)

# With this branch
 Test Files  14 passed (14)
      Tests  59 passed (59)

# Wider session hooks
 Test Files  30 passed (30)
      Tests  352 passed (352)

@alt-glitch alt-glitch added type/perf Performance improvement or optimization comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have labels Jul 27, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the adaptive floor through the deferred view-sync path. The premise is present on current main: runFlush measures only flushQueuedDeltas() in apps/desktop/src/app/session/hooks/use-message-stream/index.ts:266-271, while updateSessionState schedules the active view sync through rAF in apps/desktop/src/app/session/hooks/use-session-state-cache.ts:311 and :261-264.

Problems

  • The added tests mock updateSessionState in apps/desktop/src/app/session/hooks/use-message-stream/delta-flush.test.tsx:60-65; they do not exercise syncSessionStateToView, which owns the rAF whose registration order is central to this change. They validate the intended arithmetic, but not the production ordering contract.

Suggested changes

  • Add a focused composed-hook test using the real useSessionStateCache update function, manually drain the view-sync and measurement rAF callbacks, and assert that the post-sync measurement controls the next adaptive gap. Keep the parked-rAF fallback coverage.

Automated hermes-sweeper review.

…it frame

scheduleDeltaFlush's adaptive floor is driven by lastFlushCostRef, but
runFlush only timed flushQueuedDeltas(), the synchronous store write.
While a session streams, syncSessionStateToView defers the $messages
publish (React commit + Streamdown re-parse) to its own rAF, so the
measured cost stayed near zero and the floor collapsed to the fixed
33ms path no matter how expensive the real commit was.

runFlush now records the write cost as a fallback, then extends the
measurement through a rAF registered after the view-sync one: it runs
in the same frame right after the deferred commit, and the rAF
timestamp marks frame start so only in-frame work is counted, not the
vsync wait. A stale callback from before a newer flush is ignored, and
a hidden renderer that never fires rAF keeps the write-cost fallback.
@Adolanium
Adolanium force-pushed the fix/desktop-stream-flush-frame-cost branch from 82cb4fe to 48f6048 Compare July 30, 2026 15:11
@Adolanium

Copy link
Copy Markdown
Contributor Author

Added the composed-hook coverage in 48f6048. A new describe block wires useMessageStream to the real useSessionStateCache update function instead of the mocked one, so the view-sync rAF inside syncSessionStateToView is the production one.

The test seeds a busy state, lets a flush run, then asserts exactly two rAF callbacks are pending in registration order: draining the first publishes the deferred $messages commit (which is what identifies it as the view-sync callback), draining the second closes the measurement frame at a simulated 60ms of in-frame work, and the next adaptive gap stretches to 3x. The parked-rAF fallback case is covered against the real cache too. Full use-message-stream suite passes 71/71.

@teknium1 teknium1 added sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/streaming Streaming responses: gateway delivery, provider wire labels Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

One PR addresses issue #72799. #72801 replaces store-write-only timing with a post-view-sync animation-frame measurement, preserving timer-driven delivery, a write-cost fallback for parked renderers, and stale-callback protection.

Related pull requests

Suggested consolidation

Keep #72801 open with a salvage path, consistent with the maintainer-bot keep_open verdict: retain the deferred-frame measurement, fallback, stale-callback guard, and real-cache ordering tests, then add integration or performance validation demonstrating that the actual React commit is included in the measured frame cost. No competing PRs or duplicates are present.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I72799(["issue #72799 (open)"])
    P72801["PR #72801 (open)"]
    P72801 -->|best fix| I72799
    class I72799 open
    class P72801 open
    class P72801 best
    class P72801 target
    click I72799 "https://github.com/NousResearch/hermes-agent/issues/72799"
    click P72801 "https://github.com/NousResearch/hermes-agent/pull/72801"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 14 kB of PR diffs, 9 kB of issue/PR text, <1 kB of discussion (1 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Salvaged as #77652 with your authorship preserved — clean pick. Verified the premise carefully: main's adaptive floor predates your PR and yours is the missing half (the cost clock stopped before the deferred React commit, so the floor collapsed to 33ms under load). No defer-forever (measurement-only deferral, 250ms cap), stale-rAF guard verified. Thank you! Closing in favor of #77652.

@Adolanium
Adolanium deleted the fix/desktop-stream-flush-frame-cost branch August 6, 2026 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: desktop adaptive stream flush measures the store write, not the React commit (floor never adapts)

5 participants