Repository navigation
fix(fetch): complete startup work within the original readiness deadline - #1584
lanyue-llk wants to merge 6 commits into
Conversation
Sequential Navigation Soak A/B✅ HEAD completed the 200-navigation resilience and memory observation. One browser process, one target, and one CDP session navigate CSDN → SegmentFault → Huaban → example.com repeatedly. This run issued 200 Common ancestor Session resilience
A public-page failure is reported separately from an unrecoverable session. The soak requires all navigation attempts, zero failed recovery, zero lifecycle/network ordering violations, and complete resource evidence. Process-tree memory
HEAD memory by 50-navigation quarter
Raw reports and per-navigation boundary samples: workflow run and full Public-site timing and memory are observational. Use the A/B deltas and trend shape as evidence, not as a deterministic performance threshold. |
CI Regression ReportSource CI run · source state at render:
Release regression — ✅ HEAD/base failures 0/0; raw binary +0.012237%Package and image size
Startup latency and PSS
Concurrency matrix
Frontend differential — ✅ 1,020/1,020 cases matched; 0 issues
Agent episodes · Moli vs Chromium — ✅ 8/8 Moli episodes passed; 0 failures
Runtime and CDP session contracts — ✅ 26 contract cases; 0 failures
CDP smoke — ✅ 52/52 groups passed; 622 scenariosWorkers: 4 · cumulative group time: 199.37 s · failed groups: 0
WebMainBench · 545 pages — ✅ 545/545 completed; 0 unexpected failures
Frozen HTML · Linux · Diagnostics: All artifact fields are parsed by the trusted default-branch renderer; missing or invalid inputs remain visible as unavailable. |
Spider Bench A/B✅ All benchmark browser service runs completed; results are informational. Public HEAD: 184 / 240 rows; 38 / 48 sites produced rows; 10 unexpected empty sites. ✅ Deterministic fixture contract is clean. Common ancestor Public 48-site run · informational
Public-site content, timing, and memory are noisy. This single A/B run reports evidence only; site outcome counts explain missing rows without treating them as a deterministic regression. CPU and memory timelinesThese are bounded 40-point views of the same complete process-tree samples used by the HTML report. CPU uses 100% per occupied logical core; memory lines are RSS first and PSS second when complete PSS samples are available. Basexychart-beta
title "Base CPU"
x-axis "Elapsed seconds" [0.535, 1.036, 2.537, 3.538, 4.539, 5.54, 6.04, 7.042, 8.043, 9.043, 10.544, 11.546, 12.546, 14.048, 15.05, 16.05, 17.551, 18.552, 19.554, 21.057, 22.059, 22.559, 23.06, 24.061, 25.562, 26.563, 27.565, 29.067, 30.069, 31.071, 32.572, 33.573, 34.574, 35.575, 37.075, 38.077, 39.079, 40.578]
y-axis "CPU percent" 0 --> 200
line [3.99, 6, 121.8, 109.66, 159.77, 175.86, 113.92, 115.86, 121.86, 97.94, 115.95, 141.68, 136, 95.75, 15.99, 125.76, 6, 40, 107.77, 107.82, 125.86, 99.92, 111.72, 101.82, 63.95, 133.91, 111.84, 97.79, 113.87, 9.99, 131.99, 101.91, 87.93, 10, 103.86, 125.74, 13.99, 124.14]
xychart-beta
title "Base memory: RSS then PSS"
x-axis "Elapsed seconds" [0.034, 0.535, 1.036, 2.537, 3.538, 4.539, 5.54, 6.04, 7.042, 8.043, 9.043, 10.544, 11.546, 12.546, 14.048, 15.05, 16.05, 17.551, 18.552, 19.554, 21.057, 22.059, 22.559, 23.06, 24.061, 25.562, 26.563, 27.565, 29.067, 30.069, 31.071, 32.572, 33.573, 34.574, 35.575, 37.075, 38.077, 39.079, 40.578, 41.117]
y-axis "Memory MiB" 0 --> 500
line [13.05, 54.08, 63.27, 114.86, 119.45, 146.73, 182.76, 203.19, 194.98, 163.71, 151.81, 132.21, 148.55, 147.65, 135.17, 125.65, 158.16, 117.63, 121.3, 153.98, 141.31, 179.59, 248.29, 236.2, 187.96, 130.71, 152.6, 142.04, 138.2, 146.23, 141.93, 200.99, 223.4, 219.87, 148.16, 151.93, 159.38, 128.28, 172.46, 0]
line [9.64, 50.5, 59.69, 111.09, 115.86, 143.08, 179.2, 199.74, 191.41, 160.4, 148.28, 128.62, 145.71, 144.07, 131.54, 122.09, 154.54, 114.16, 117.7, 150.37, 137.68, 176.03, 244.78, 231.77, 184.34, 127.09, 148.98, 138.53, 134.64, 142.55, 138.29, 197.34, 220.24, 213.75, 144.52, 146.66, 155.48, 124.63, 168.85, 0]
HEADxychart-beta
title "HEAD CPU"
x-axis "Elapsed seconds" [2.035, 4.035, 5.537, 6.038, 8.042, 10.043, 12.046, 14.05, 16.051, 18.055, 19.057, 20.059, 22.062, 24.065, 26.068, 28.071, 30.071, 32.072, 34.075, 36.576, 38.576, 40.579, 42.58, 44.582, 46.585, 48.588, 50.59, 51.59, 52.592, 54.594, 56.597, 58.597, 60.599, 62.601, 64.605, 66.607, 68.61, 70.613]
y-axis "CPU percent" 0 --> 200
line [91.91, 144.08, 179.76, 113.89, 119.75, 135.98, 105.85, 97.83, 111.8, 3.99, 0, 2, 2, 2, 0, 0, 0, 0, 0, 18, 17.99, 18, 17.98, 119.88, 53.92, 53.86, 122.01, 109.91, 131.83, 123.9, 133.9, 99.88, 77.98, 117.83, 2, 13.96, 123.85, 71.97]
xychart-beta
title "HEAD memory: RSS then PSS"
x-axis "Elapsed seconds" [0.034, 2.035, 4.035, 5.537, 6.038, 8.042, 10.043, 12.046, 14.05, 16.051, 18.055, 19.057, 20.059, 22.062, 24.065, 26.068, 28.071, 30.071, 32.072, 34.075, 36.576, 38.576, 40.579, 42.58, 44.582, 46.585, 48.588, 50.59, 51.59, 52.592, 54.594, 56.597, 58.597, 60.599, 62.601, 64.605, 66.607, 68.61, 70.613, 72.206]
y-axis "Memory MiB" 0 --> 500
line [12.9, 107.66, 137.86, 181.29, 204.57, 161.81, 149.1, 152.63, 140.79, 171.93, 147.98, 147.98, 147.05, 147.05, 148, 147.05, 147.05, 147.95, 147, 147.96, 146.96, 147.96, 146.96, 146.96, 161.63, 128.21, 162.86, 167.56, 256.85, 181.47, 215.81, 169.64, 137.88, 143.08, 211.25, 225.56, 145.01, 167.32, 137.79, 0]
line [9.48, 103.88, 134.2, 177.64, 201.04, 158.1, 145.39, 148.97, 137.08, 168.31, 144.23, 144.24, 143.3, 143.3, 144.51, 143.56, 143.56, 144.59, 143.64, 144.59, 143.6, 144.6, 143.6, 143.6, 158.34, 124.78, 159.65, 164.22, 252.75, 177.91, 212.54, 166.21, 134.39, 139.6, 207.71, 222.08, 141.53, 163.92, 134.34, 0]
Deterministic fixture · required contractThe fixture exercises 15 routes: 8 are expected to emit rows and 7 intentionally exercise empty, loading, or timeout behavior.
Full HTML, JSON, CSV, logs, and page snapshots: workflow run and full The public-web A/B is informational. The exact HEAD fixture contract runs as a separate required CI check; timing and public-site content deltas remain non-blocking. |
a06539d to
fa26292
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
复核提交 fa26292d0c79，未发现本次改动新增的明确阻断问题。
Done 补跑 load 后已就绪任务有实际用途，16 轮上限也保留了它作为有限完成策略的含义。调用方的 FetchDeadline.wait 包住这段补跑，导航替换继续携带原请求阶段；生命周期让出执行机会时保留普通任务源的公平性计数，能避免 Worker 消息积压反复重置计数而饿死定时器。DomStable 增加的检查对应仍可能产出页面内容的脚本、子文档和 Worker 启动，未扩大到 Worker 的整个存活期。
本次核对了上述调用路径及新增场景的断言，没有把 PR 描述中的历史整合测试结果算作当前提交的测试。本机缺少原生开发工具，未运行该提交的 V8/网络集成测试。
fa26292 to
f18cca0
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
This PR makes Done drain a bounded tail of already-runnable post-load work (load-handler zero-delay tasks, etc.) under the caller's original FetchDeadline. It also widens domstable to wait for parser async scripts, child-document lifecycle and Worker bootstrap, stops lifecycle turns from resetting ordinary-source fairness, and fixes a dropped completion when a pending location navigation sees a replaced VM.
Assessment: request changes. Mechanically, the deadline plumbing is sound. The host wraps the renderer command in deadline.wait(...), so a renderer turn that blocks (a busy script, a slow navigation) hits a typed FetchReadinessTimeout at the original absolute deadline, and every Done entry point (fetch_page_with_wait_until_deadline, fetch_request_document_..._deadline, and the lifecycle-decider/CLI path through lifecycle_fetch.rs) now reaches the drain. My concerns are the contract it creates and how the renderer side uses its deadline:
Doneis the CLI default. It changes from "succeed at Load" to "fail outright if the post-load tail outlasts the remaining budget", while a tail that outlasts the turn budget silently succeeds partially. See the inline comment onmod.rs.- When the renderer hits its own copy of the deadline it replies
Unit(success). The typed failure therefore depends on host and renderer clock skew rather than an explicit renderer error, which is unlike the networkidle/domstable owner waits. - The turn budget counts completer re-dispatches (any page wake), not page turns, and comes from adding two counters measured in different units.
Not anchorable / general:
- The deadline tests (
fetch_done_deadline_includes_busy_post_load_script,..._post_load_navigation) use one 1200 ms blocking task or a 1200 ms navigation. They exercise only the host-sidetimeout_at. Nothing tests the renderer-sideInstant::now() < deadlinebranch: a chain of short tasks spanning the deadline should fail with a typed timeout and must not return Ok. Nothing tests turn-budget exhaustion either: a chain longer than 16 hops shows what Done returns. - The
vm_creation_idmismatch change inowner_loop.rs, which now continues post-parse lifecycle instead ofBackgroundComplete(Ok(())), is a real bug fix: the previous code droppedPublishedPageCreation/CompletePageCreationcompletions. The PR body does not mention it, and no test is identified that fails without it. Please call it out and point at the covering fixture. - Docs:
RenderedDomWaitUntil::Doneand the CLI--wait-untilhelp still say nothing about the new strict post-load phase or its failure mode, even though skills and READMEs recommenddoneas the default. complete_post_load_ready_tasks_turntakes the entry and restores it straight away just to validate the token, then takes it again to complete. That is a small extra round trip; consider one take.- CI: the CI Regression Report shows Release regression "artifact unavailable or invalid", and the Sequential Navigation Soak failed on infrastructure. There is no release-regression or soak signal for a change that alters the default fetch mode, so please re-run before merge.
Verdict: changes requested (posted as a comment because GitHub does not allow requesting changes on your own PR).
Automated review by Claude Code.
5f09b41 to
226842c
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Re-review of the updated version (89851ddd8)
Re-review of 89851dd. After Load, done drains up to 16 already-runnable Page turns under the caller's original deadline. The renderer now reports an explicit Idle / TurnBudgetExhausted / DeadlineReached outcome. DomStable additionally waits for parser async scripts, child-document lifecycle and Worker bootstrap.
Verdict: comment (approvable once the deadline-precedence item below is addressed and Nextest/release CI finish green). The previous major issue (strict versus best-effort Done) is resolved via option (b): deadline expiry stays a typed failure, budget exhaustion returns the page with a warn!, and the docs and CLI help describe this. Every inline finding from last round is fixed or has a valid pushback.
Previous findings
| # | Finding | Status | Evidence |
|---|---|---|---|
| 1 | [major] Done strict on time, silent on turn budget | FIXED | mod.rs wait_for_page_readiness_with_deadline: TurnBudgetExhausted warns and returns Ok, DeadlineReached maps to FetchReadinessTimeout. Doc comment, RenderedDomWaitUntil::Done and CLI help updated |
| 2 | [minor] Renderer deadline replied Unit (clock skew) |
FIXED | Explicit RendererPostLoadReadyCompletion::DeadlineReached. The zero-deadline test asserts it. One new precedence issue, see below |
| 3 | [minor] Budget counted dispatches, not turns; duplicate kick | FIXED | PageTurnScheduler::admitted_turn_count increments on scheduled checkout, and the kick is sent only when signal_scheduler is set (first dispatch). The test does not prove it, see below |
| 4 | [minor] "Derived" magic 16, no diagnostic | FIXED | MAX_POST_LOAD_READY_PAGE_TURNS is an explicit policy constant, and the host warns on exhaustion |
| 5 | [minor] Worker bootstrap in pending_subresource_request_count |
FIXED | resource_loading.rs no longer in diff. The term is only in the page_dom.rs DomStable predicate, and the diagnostics test asserts pending_subresource_requests == 0 while loading |
| 6 | [minor] DomStable waits full budget on hanging child | PUSHBACK ACCEPTED | fetch_with_domstable_bounds_a_child_that_does_not_finish_in_time shows the result is bounded by the caller deadline and returns the parent page. The latency tradeoff is now explicit |
| 7 | [minor] unreachable!() in inline route |
FIXED | page_commands.rs returns the sibling Err pattern, covered by direct_page_vm_wait_commands_fail_closed |
| 8 | [nit] dedicated_worker_loading_count_for_diagnostics alias |
FIXED | Renamed in place, no wrapper |
| 9 | [nit] Duplicated Page/Raw readiness match in lifecycle_fetch.rs |
FIXED | Shared finish_fetched_document_readiness (no author reply on that thread, but fixed in code) |
| S1 | Tests for renderer deadline branch / budget exhaustion | FIXED | post_load_ready_task_completion_honors_zero_deadline, ..._counts_real_page_turns (the latter is weak, see below) |
| S2 | vm_creation_id mismatch fix not described / no covering test named |
NOT FIXED | The PR body still does not mention the pending-location-navigation completion fix (owner_loop.rs, now with target_stage) or name its covering fixture |
| S3 | Docs for Done failure mode | FIXED | See #1 |
| S4 | Take/restore/take round trip in complete_post_load_ready_tasks_turn |
NOT FIXED | Still takes and immediately restores the entry at page_commands.rs:202 (nit, see inline) |
| S5 | CI release-regression / soak unavailable | OPEN | On 89851dd, Nextest and both release builds are still queued. The Soak, Spider Bench and CI Regression comments are for the old head 5f09b41 and give no signal for this head |
New findings
- [minor]
DeadlineReachedtakes precedence overIdle. Combined with millisecond flooring, an idle page failsdonewith less than 1 ms of budget left (inline,page_commands.rs:243). - [minor]
post_load_ready_task_completion_counts_real_page_turnspasses with or without the admitted-turn counter, so the fix for #3 is untested (inline,tests.rs:560). - [nit]
RenderedDomWaitUntil::Donedoc omits the budget-exhaustion outcome (inline,mod.rs:181). - [nit] Repeat of S4 (inline,
page_commands.rs:202).
Merge: git merge-tree --write-tree origin/main refs/review/r2-1584 is clean (14d3c08). Commit stack (4 commits) is coherent. No empty or unrelated commits.
Automated review by Claude Code.
1b974af to
da09361
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Final re-review (P0/P1 only) (da0936177)
Final review of da09361. Verdict: approve. No P0/P1 issues remain. Nextest and both release builds are still pending, so merge only after they pass.
Previous P0/P1-level findings
| # | Finding | Status | Evidence |
|---|---|---|---|
| R1-1 | [major] Done strict on time, silent on turn budget | FIXED (r2) | mod.rs:836-866: Idle returns Ok, TurnBudgetExhausted warns and returns Ok, and DeadlineReached maps to FetchReadinessTimeout |
| R1-2 | Renderer deadline reply could become success | FIXED (r2) | Explicit RendererPostLoadReadyCompletion::DeadlineReached |
| R2-1 | Idle page with <1 ms left fails done: deadline checked before readiness, plus host ms flooring |
FIXED | page_commands.rs:237-245 now decides Idle whenever readiness is not Runnable, before the deadline check. wait_support.rs:22-25 rounds the remainder up with as_micros().div_ceil(1000), so the renderer deadline no longer falls before the host deadline. Covered by post_load_ready_task_completion_reports_idle_before_an_expired_deadline (tests.rs:559, /static with Duration::ZERO, expects Idle). The always-runnable zero-deadline test still expects DeadlineReached |
| R2-2 | Turn-counter fix untested | FIXED | tests.rs:603-639 arms a setTimeout(0) chain. 16 hops gives Idle with the chain complete, and 17 hops gives TurnBudgetExhausted with exactly 16 hops done. The renderer has no timer-nesting clamp, so every hop stays immediately runnable. SpentWake no longer consumes budget (owner_local_store/mod.rs:1547) |
New findings
No P0/P1 issues found. I checked the following for hangs, false timeouts and partial-page-as-success:
- Removing the validation take/restore leaves a missing or retiring page on the
take_entry_for_commandErrpath. It does not become a falseIdle. - The continuation is bounded by
ready_at: deadline. - A Blocked or Idle page at expiry now reports
Idle. - The only remaining false-timeout window is a reply that arrives after the host's own absolute deadline (
FetchDeadline::wait). That is inherent to the accepted strict-deadline contract. - Excluding spent wakes from the budget cannot livelock past the deadline.
Merge: git merge-tree --write-tree origin/main(14d3c08cc) refs/review/f-1584 is clean (09ac89068), and GitHub reports MERGEABLE.
CI on da09361: Format, Clippy, CDP smoke, Node, Python, Windows and Resolve revisions pass. Nextest, Build release HEAD and Build release common-ancestor are pending.
Verdict: approve (posted as a comment; self-approval is not allowed).
Automated review by Claude Code.
|
Follow-up on the current-head Nextest failure: the product behavior was correct; the 17-hop witness was scheduler-order-sensitive because one ready task can run between the expression command and the completion command. Commit 532fbf0 replaces it with a 64-hop chain and asserts the reader-facing contract (finite completion returns with runnable work still pending). The previously failing test now passes locally: |
Problem
--wait donecould return at Load before a zero-delay task queued by the load handler updated the page.domstablecould also settle while parser-owned async scripts, child-document startup, or Worker bootstrap still had work in progress. The scheduler reset ordinary-source fairness across lifecycle turns, allowing an older Worker backlog to delay a timer or network task that was already ready.A pending script navigation could also lose its completion when the active Page VM was replaced before the original continuation resumed.
Change
After Load,
donenow consumes up to 16 actual Page turns that are already runnable. Spent wake notifications do not consume that budget. The renderer reports one of three outcomes: idle, turn budget exhausted, or deadline reached.The caller's original fetch deadline covers the complete operation. An idle page succeeds even at the boundary, while a still-runnable page reports deadline expiry as a typed fetch failure. A positive sub-millisecond remainder is rounded up when sent to the renderer so its local copy cannot expire before the host deadline. Turn-budget exhaustion returns the current page and emits a warning.
The same readiness finish path serves normal fetches and lifecycle-decider fetches. Worker bootstrap contributes to
domstableruntime readiness without being counted as a network request. A child document that does not finish remains bounded by the caller's existingdomstabledeadline.Pending location navigation now follows a replacement VM through the requested lifecycle stage instead of dropping the completion. The HTTP-redirect script-navigation fixture covers that replacement path.
Reproduce and verify
The focused tests cover:
donetail;domstablewithout changing the public pending-request count;Local execution on macOS is blocked before compilation because Apple Command Line Tools are unavailable.
cargo fmt --all -- --checkandgit diff --checkpass. CI runs the Rust suites.Browser Eval integration
Build this branch and register that exact executable, version output, and SHA-256 as
moli-candidate. Browser Eval can then run the frozen dynamic-page cases through the certified native-fetch entry point:These public cases are cross-PR integration references, not causal proof for this PR. The profile runs them with
domstable:dynamic-015anddynamic-019also exercise #1592's independent frame-Markdown stack, whiledynamic-104also exercises #1593's ResizeObserver change. The focused fixtures above isolate the readiness behavior changed here.Dependency
Independent change targeting
main; no other open PR is required.