fix(web-ai): bound assistant DOM reads by the polling deadline - #90
Closed
dbc-hbin wants to merge 1 commit into
Closed
fix(web-ai): bound assistant DOM reads by the polling deadline#90dbc-hbin wants to merge 1 commit into
dbc-hbin wants to merge 1 commit into
Conversation
…-jun#88) ChatGPT polling could stay alive with no progress well past `--timeout`. The poll loop only re-checked its deadline at the `while` boundary, then awaited `readAssistantMessages()`, which called `page.evaluate()` with no per-call bound. Playwright's `page.evaluate()` accepts no timeout option, so when assistant message extraction stalled on a very large conversation, control never reached the next deadline check: the process and the session lock stayed alive while the stderr heartbeat went silent. - add `withAssistantReadTimeout()` and `resolveAssistantReadBudgetMs()`; every assistant read is raced against the smaller of the remaining command deadline and a per-read ceiling - bound the `page.evaluate()` path, the locator fallback, and the post-timeout recovery read, which otherwise re-hung the command it exists to rescue - add `readAssistantTextsAfterIndex()`: count turns in-page but serialize only the turns after the baseline, instead of re-serializing the whole conversation on every 500ms tick - keep emitting the poll heartbeat while reads stall, since silence with a live process was the original symptom - report a stalled read distinctly from ongoing generation via an `assistant-dom-read-timeout:<n>` warning on the timeout envelope A read that exceeds its budget now retries on the next tick and, at the deadline, returns the existing recoverable `provider.poll-timeout` envelope, so `poll` and `sessions resume` behavior is unchanged. Exhausting the budget at the loop boundary is treated as the deadline itself, not as a DOM-read failure, so ordinary timeouts are not mislabeled. Tests: new `test/unit/web-ai-assistant-read-deadline.test.mjs` drives `pollWebAi` with a `page.evaluate()` that never resolves and asserts the command still honors its deadline and reports the stall; it also covers the budget helpers and proves historical turns are no longer re-serialized. Verified that removing the bound makes the poll test hang until the runner's own timeout.
Author
|
Superseded by #89, which now carries this fix as its own commit ( Merging them separately would have required resolving a Closing to keep review in one place. No content was dropped. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #88
Problem
ChatGPT polling could stay alive with no progress well past
--timeout.pollWebAichecks its deadline only at thewhileboundary, then awaits:which calls:
Playwright's
page.evaluate()accepts no timeout option (onlylocator.evaluatetakes one). So when assistant-message extraction stalled on a very large conversation, control never reached the next deadline check. The process and the session lock stayed alive while the stderr heartbeat went silent, which matches the reported symptom:sessions doctorreported a valid target, a healthy command-lock heartbeat, and no CDP or login failure.The same function also read and cleaned every top-level assistant message on each 500 ms tick, then sliced off everything before
baseline.assistantCount— so long conversations paidinnerTexton the whole history to obtain one new answer.Changes
withAssistantReadTimeout()andresolveAssistantReadBudgetMs()inchatgpt-response-dom.mjs; each assistant read is raced against the smaller of the remaining command deadline and a per-read ceilingpage.evaluate()path, the locator fallback, and the post-timeout recovery read inchatgpt-response-observer.mjs, which otherwise re-hung the command it exists to rescuereadAssistantTextsAfterIndex(): count turns in-page but serialize only the turns at/after the baselineassistant-dom-read-timeout:<n>on the timeout envelope so a stalled DOM read is distinguishable from provider generation still streamingBehavior
A read that exceeds its budget retries on the next tick; at the deadline the existing recoverable
provider.poll-timeoutenvelope is returned, sopollandsessions resumesemantics are unchanged.Two details worth reviewing:
The trimmed read falls back to the existing full read whenever it observes no turns, which keeps pages that cannot serialize the object argument on the previous behavior.
Tests
New
test/unit/web-ai-assistant-read-deadline.test.mjsreproduces the defect deterministically without ChatGPT, as suggested in the issue: it drivespollWebAiwith apage.evaluate()that never resolves and asserts the command still honors its own deadline, returns the recoverable timeout envelope, and reports the stall.It also covers the budget helpers (clamping, rejected reads, sentinel) and proves via getter instrumentation that historical turns are no longer re-serialized.
I verified the test actually catches the bug: with the bound removed, that poll test hangs until the runner's own timeout instead of passing.
One existing fake page in
web-ai-provider-session.test.mjskeyed its answer sequence off a rawevaluatecall count, so it was updated to key off the assistant-read argument shape instead. The assertions are unchanged.Verification
test/integration/post-action-smoke.test.mjsandself-heal-smoke.test.mjsfail in my environment before this change as well, because no Chromium is installed locally; they are unrelated to this diff.I have not reproduced the fix against a live 40–60 KB multi-turn ChatGPT conversation, so the real-world stall threshold is still unverified; the deadline behavior itself is covered deterministically.