Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38dd066e2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SCALE_URL="$URLS" SCALE_TAG="$TAG" SCALE_OUT="$RESULTS/scale" \ | ||
| SCALE_UFFD_MODE="$UFFD_MODE" SCALE_UFFD_PREFETCH="$UFFD_PREFETCH" \ | ||
| SCALE_CONTROL_URL="${SCALE_CONTROL_URL:-${URLS%%,*}}" \ | ||
| SCALE_CONTROL_RESOLVE_ALL_TO="${SCALE_CONTROL_RESOLVE_ALL_TO:-127.0.0.1}" \ | ||
| make -C "$REPO" bench-chromium-scale 2>&1 | tee "$LOGDIR/run.log" || run_rc=$? |
There was a problem hiding this comment.
When MEASURE=scale completes successfully, this target writes raw artifacts under $RESULTS/scale, but it never creates the $RESULTS/analysis.json that write_dns_evidence exclusively reads to obtain the measured run_id. The evidence writer therefore marks every scale run unclean, and the final verdict check exits 1, so the new advertised campaign mode can never complete successfully. Analyze the scale output or bind its provenance/status directly before writing the evidence.
AGENTS.md reference: bench/chromium/AGENTS.md:L262-L268
Useful? React with 👍 / 👎.
reqscale.py took one --url, served its UFFD backend in copy mode whatever the corpus used, and its host control Chromium rendered the same --url, which for a corpus page would resolve on the live internet. No target ran it against the corpus replay. bench/chromium/reqscale.py: - --url may be a comma-separated list. request_url() cycles it by pair index, so the FILE and UFFD halves of a pair render the same page, and each request record carries the url it rendered. - A list needs --control-url, one URL, for the host control. - --control-resolve-all-to IP maps every name the control Chromium resolves to IP (--host-resolver-rules=MAP * IP); provenance records it as host_control.resolve_all_to. - --uffd-mode (copy|minor, default copy) and --uffd-prefetch (on|off, default on) reach `fcvm snapshot serve`. - The control Chromium's and the memory server's argv are built by command() methods, so they can be checked without starting either. bench/chromium/reqscale_analyze.py: host_control's key set includes resolve_all_to. Makefile: SCALE_UFFD_MODE, SCALE_UFFD_PREFETCH, SCALE_CONTROL_URL and SCALE_CONTROL_RESOLVE_ALL_TO reach reqscale.py. bench/chromium/corpus_campaign.sh: MEASURE=scale replaces the serial run with make bench-chromium-scale over the 14 corpus URLs, inside the same golden, verify, diag and DNS-evidence bracket, in the campaign's UFFD mode, with the control rendering the corpus's first URL against this host's replay server (127.0.0.1). Rates, bursts, seed, gates and the control binary come from the caller's SCALE_* variables. MEASURE=scale refuses ENGINE=webkit and BACKEND other than uffd. Tests, each observed failing on the parent: - CorpusPairing, ConcurrentRequestRecords.test_a_corpus_request_renders_its_pairs_url_and_says_so - ControlResolverRule, ServeMode - PlanOnlyCli: a list without --control-url, a list as the control URL, and a non-IPv4 resolver target are refused - DnsBrackets.test_measure_scale_runs_the_open_loop_benchmark_over_the_corpus Tested: python3 -m unittest discover -s bench/chromium -p 'test_*.py' (1070 tests, OK). `make -n -o build bench-chromium-scale ...` shows the new flags on the reqscale.py command line.
…check the restore path From a Codex review of the previous commit. bench/chromium/reqscale.py: - The URL list is part of ScheduleConfig and the durable schedule (urls, url_selection), so the analyzer's rebuild checks it. - A list of more than one page needs every rate's scored window to hold whole cycles of it: at 2 rps 120 scored pairs gave eight of 14 pages 9 renders and six 8, the same bias in every burst, so the rate curve was confounded with the page mix. Rates that are multiples of 1.4 rps fit a 14-page corpus. - require_serve_mode(): the memory server must run the requested mode; fcvm coerces minor to copy for NV2 snapshots. - file_restore_refusal(): a hugepage snapshot, a snapshot with a kernel profile (NV2), or FCVM_FORCE_UFFD makes `fcvm snapshot run` restore through an implicit UFFD server, so the FILE arm would be UFFD; the run is refused. - Provenance schema v2 (host_control.resolve_all_to). bench/chromium/reqscale_analyze.py: - Rebuilds the schedule with its urls, and each request must carry the url of its pair's slot. - Requires provenance v2; resolve_all_to must be null or canonical IPv4. - corpus_dns_gate(): a corpus run (more than one url, or a control resolver rule) is publishable only beside a clean dns-evidence.json that names its run. Otherwise publishable is false and publication_blocked_by says why. The analysis also lists the corpus. bench/chromium/corpus_campaign.sh: MEASURE=scale failed every run, because write_dns_evidence read $RESULTS/analysis.json, which a scale run never writes. The scale branch analyzes into scale/analysis-pre-evidence.json, the evidence names its run from ANALYSIS_JSON, and after a clean verdict the run is analyzed again into scale/analysis.json, which must say publishable or the campaign fails. Tests, each observed failing on the previous commit (SpawnedArgv on main): - CorpusPairing: every scored window of every burst renders each page equally often per backend; a cycle-splitting rate is refused - AnalyzerHoldsTheCorpus: a request that rendered another page, the DNS gate, the resolver field - RestorePathIsWhatItSays: the FILE-arm refusal and the serve mode - SpawnedArgv: the argv start() actually hands Popen for the memory server and the control Chromium (replaces the helper-only tests) - DnsBrackets.test_measure_scale_runs_the_open_loop_benchmark_over_the_corpus now checks both make calls and the evidence's analysis path; the fake make records each call's environment by target Tested: python3 -m unittest discover -s bench/chromium -p 'test_*.py' (1088 tests, OK).
3dd8c2e to
5404b32
Compare
38dd066 to
7d86279
Compare
This branch has not been deployed
1 participant
Stacked on: bench/request-cpu
The render benchmark page says throughput under load is not measured on the corpus.
reqscale.py, the open-loop harness, took one--url. It served its UFFD backend in copy mode whatever mode the corpus runs used. Its host control Chromium rendered that same URL, which for a corpus page would resolve on the live internet. No target ran it against the corpus replay.Contract:
make bench-chromium-corpus MEASURE=scale SCALE_…runs the open-loop benchmark over the 14 corpus URLs. It runs inside the campaign's golden, verify, diag and DNS-evidence bracket, in the campaign's memory-server mode, with a host control that renders against the local replay server.Downstream impact:
reqscalegains optional flags, and each request record gains aurlfield. Single-URL runs behave as before.Changes
reqscale.py--urlmay be a comma-separated list.request_url()cycles it by pair index, so the FILE and UFFD halves of a pair always render the same page, and each record names its URL.--control-url, exactly one URL.--control-resolve-all-to IPadds--host-resolver-rules=MAP * IPto the control Chromium. Provenance records it ashost_control.resolve_all_to.--uffd-mode(copy or minor, default copy) and--uffd-prefetch(on or off, default on) reachfcvm snapshot serve.command()methods, so they are testable.Other files
reqscale_analyze.py: the host-control key set includesresolve_all_to.Makefile: addsSCALE_UFFD_MODE,SCALE_UFFD_PREFETCH,SCALE_CONTROL_URLandSCALE_CONTROL_RESOLVE_ALL_TO.corpus_campaign.sh:MEASURE=scaleswaps the serial run forbench-chromium-scaleover the corpus. The control renders the corpus's first URL against127.0.0.1, and rates, bursts, seed and gates come from the caller'sSCALE_*. It refuses WebKit and non-UFFD backends.Evidence
Each new test was observed failing on the parent:
CorpusPairing,ControlResolverRule,ServeModeConcurrentRequestRecords.test_a_corpus_request_renders_its_pairs_url_and_says_soPlanOnlyCli(two refusal tests)DnsBrackets.test_measure_scale_runs_the_open_loop_benchmark_over_the_corpusreqscale.pyhas never run end to end on any host; its first run is the pbox campaign that follows. Fixes from that run land here before merge.