Conversation
The host-container control started a timing wrapper (`python3 -`) and a `python3 cdpdrive.py` process for every rep, so each host wall_ms carried two interpreter start-ups the VM arm never pays: reqbench.py imports cdpdrive once and times one drive() call per request. The render benchmark page charged 60.7 ms of the host container's wall time to that wrapper. bench/chromium/hostcdp.sh: - One `python3 -` process imports $HERE/cdpdrive.py and loops over every rep, timing each cdpdrive.drive() call on the monotonic clock. Its arguments are what `cdpdrive.py ADDRESS URL --format jpeg --nav-timing` parsed to. - Each record keeps its fields and is written before its rep is judged: a non-numeric or unreadable 1-minute load still refuses with exit 5, and a failed rep with exit 4. The driver field holds the whole result JSON; the old loop kept its last 2,000 characters, which compare.py then had to parse as JSON. - run.json records "driver_process": "in-process"; records without it started a process per rep. - The loop lives in hostcdp.sh, which is hashed before and after every run, so the sealed source lists and runtime manifests are unchanged. Tests: - test_hostcdp_corpus.write_python_shim logs every python3 start and hands the in-process loop a stub cdpdrive module; test_hostcdp and the CPU budget tests use it. - HostCdpDrivesInProcess.test_no_rep_starts_an_interpreter fails on main: reps ran cdpdrive.py as their own program and python3 starts grew with REPS. - test_corpus_mem's HostCdpProducer moves its per-rep driver actions (load file changes, runtime tamper, a leftover descendant) into the stub's drive(). test_numeric_output_from_a_failed_load_read_is_invalid is removed with its cut shim branch: it covered `cut` exiting 9 while printing a number, and the per-rep read no longer runs cut. The start-of-run gate still does and keeps its status=9 test. - The run.json metadata test pins driver_process; it fails without it. Tested: python3 -m unittest discover -s bench/chromium -p 'test_*.py' (1053 tests, OK).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe host CDP harness now runs scheduled repetitions through one Python process that imports the driver once. It records each repetition’s result and load reading. Tests use a shared Python shim and check that interpreter starts do not increase with repetition count. ChangesHost CDP driver execution
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant HostHarness as hostcdp.sh
participant PythonRunner as Python runner
participant Cdpdrive as cdpdrive
HostHarness->>PythonRunner: Start in-process driver loop
PythonRunner->>Cdpdrive: Import driver once
loop Each scheduled URL
PythonRunner->>Cdpdrive: Call drive() and time the repetition
Cdpdrive-->>PythonRunner: Return driver result
PythonRunner->>HostHarness: Write and flush repetition record
end
PythonRunner-->>HostHarness: Exit with driver or measurement status
Merge Risk: ⚪ Minimal · up to No identified issue blocks the in-process driver change from merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The reviewed paths preserve fresh CDP sessions, run ownership checks, and withdrawal of failed runs. No new security boundary bypass was established. Broader deployment exposure remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
…e driver process From a Codex review of 8b2778a. bench/chromium/hostcdp.sh: only drive()'s Exception was caught, so a KeyboardInterrupt, a non-dict result or a JSON encoding failure ended the run before the rep's row was written. The per-rep wrapper process the loop replaced still wrote a failed row when its child died. The loop now catches everything drive() raises, writes the row, and re-raises an interrupt (refused, exit 5); a non-dict result is a failed rep (exit 4), and the row's JSON falls back to repr for values it cannot encode. bench/chromium/compare.py: the host output carries driver_process. Records from before in-process driving have no field and are labelled "per-rep subprocess", so a comparison built on one says that its host wall_ms includes interpreter start-up. Tests, each failing on 8b2778a: - HostCdpDriverFailures.test_an_interrupt_writes_the_row_and_refuses - HostCdpDriverFailures.test_a_result_that_is_not_a_dict_is_a_failed_rep - ComparePublicationGate.test_a_complete_compatible_host_is_compared (driver_process)
The host-container control (
hostcdp.sh) started a timing wrapper (python3 -) and thenpython3 cdpdrive.pyfor every request, so each hostwall_msincluded two interpreter start-ups. The VM arm never pays that:reqbench.pyimportscdpdriveonce and times onedrive()call per request (reqbench.py:3172,:3270). The render benchmark page charges 60.7 ms of the host container's wall time to this wrapper.Contract: the host control times one in-process
cdpdrive.drive()per request. Records keep their fields, and its refusals keep their exit codes.run.jsonsays which way a record was driven.Downstream impact: host-control records produced after this merge, which are not comparable to earlier ones on
wall_ms. No VM-arm code changes.Changes
bench/chromium/hostcdp.shpython3 -process importscdpdrive.pyand runs every rep, timing eachdrive()on the monotonic clock. It passes the argumentscdpdrive.py ADDRESS URL --format jpeg --nav-timingparsed to.driverfield holds the whole result JSON. The old loop kept only its last 2,000 characters, whichcompare.pythen parses as JSON.run.jsongains"driver_process": "in-process". Records without it started a process per rep.hostcdp.sh, which is hashed before and after every run. The sealed source lists and runtime manifests are unchanged.Tests
test_hostcdp_corpus.write_python_shimlogs everypython3start and hands the in-process loop a stubcdpdrivemodule.test_hostcdpand the CPU-budget tests use it.HostCdpDrivesInProcess.test_no_rep_starts_an_interpreterfails on main: reps rancdpdrive.pyas their own program, andpython3starts grew withREPS.test_corpus_mem'sHostCdpProducerharness moves its per-rep driver actions (load-file changes, runtime tamper, a leftover descendant) into the stub'sdrive().test_numeric_output_from_a_failed_load_read_is_invalidis removed along with itscutshim branch. It coveredcutexiting 9 while printing a number, and the per-rep load read no longer runscut. The start-of-run gate still does, and keeps itsstatus=9test.run.jsonmetadata test pinsdriver_processand fails without it.Evidence
The measured rerun of the host control is part of the pbox campaign that follows this PR. This box's load was 34 from other work, above the control's 1.0 gate.
Summary by CodeRabbit