Skip to content

observe(api,bench): carry process CPU on every construction operation (#1462) - #1471

Merged
DecisionNerd merged 2 commits into
observe/1462-effective-coresfrom
observe/1462-wire-ingest-phases
Sep 18, 2026
Merged

DecisionNerd merged 2 commits into
observe/1462-effective-coresfrom
observe/1462-wire-ingest-phases

Conversation

@DecisionNerd

@DecisionNerd DecisionNerd commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #1470. Base is observe/1462-effective-cores; review that one first. #1470 adds the measurement, this makes something call it.

Why

#1470 established an instrument that can be trusted. Nothing called it. This wires it into the four construction operations the receipt already delimits, so #1387's serialized-fraction budget is readable off an ordinary run instead of inferred from phase totals.

ImportCallTiming gains cpu_ns and cpu_unmeasured_calls, plus effective_cores() as their ratio against elapsed_ns. CallStart captures wall and CPU together at the four sites that previously captured only Instant::now().

seal.effective_cores() is the number the budget is read from — seal is 68–80% of ingest, and #1387 needs the serial region under ~10% on 16 threads.

effective_cores() returns None when no wall elapsed or any call's CPU could not be read, so an operation that never ran cannot be mistaken for one that ran on no CPU. The test asserts exactly that for the publish that never happens.

Two fail-closed surfaces this had to move with it

Both would have failed the ladder rather than this PR's own tests, which is why they are worth calling out.

benchmarks/schemas/certification-evidence.json declares additionalProperties: false over importCallTiming. Serializing two new fields would have failed harness validation outright.

They are added as optional, not required. Required rejects every bundle recorded before this change — including the S18–S24 evidence in clean-f80f69fe-evidence/ that the harness must still read. I tried required first; it failed three harness tests with 'cpu_ns' is a required property.

benchmarks/harness/graphforge_bench/lifecycle_runtime.py seeded its accumulator with a fixed three keys and then ran total[key] += value over whatever the receipt carried — a KeyError on any field added after it was written. Now total.get(key, 0) + value, which tolerates the next addition too.

It also surfaces construction_call_effective_cores per operation in the rung summary: omitted when CPU was unavailable, and absent entirely for historical bundles, matching how lifecycle_application_io already handles evidence predating its instrumentation.

Verification

  • graphforge-api lib: 745 passed / 0 failed, including a new end-to-end test that runs a real import and asserts begin/append/seal all report CPU and a defined effective_cores().
  • Benchmark harness via CI's own invocation (PYTHONPATH=harness uv run --locked python -m unittest): 97 passed.
  • cargo clippy --workspace -- -D warnings: exit 0. cargo fmt --all -- --check: clean. uv run ruff format --check .: clean. make pre-push-fast: passed.

What this still does not do

It reports effective cores. It does not yet report a serial fraction on the rung, because that needs the worker count the operation actually ran with, and serial_fraction() in #1470 takes it as an argument for that reason. Once #1466 lands and the default resource policy stops pinning two workers, the worker count becomes meaningful and the derivation can be surfaced. Until then effective cores is the honest figure and the derived one would be misleading.

Refs #1462

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…#1462)

The instrument added in the parent commit could measure a region; nothing called
it. This wires it into the four construction operations the receipt already
delimits, so #1387's serialized-fraction budget is readable off an ordinary run
instead of inferred from phase totals.

`ImportCallTiming` gains `cpu_ns` and `cpu_unmeasured_calls`, and
`effective_cores()` as their ratio against `elapsed_ns`. `CallStart` captures
wall and CPU together at each of the four sites that previously captured only
`Instant::now()`. **`seal.effective_cores()` is the figure the budget is read
from**, seal being 68-80% of ingest.

`effective_cores()` returns `None` when no wall time elapsed or any call's CPU
could not be read, so an unrun or unmeasured operation cannot be mistaken for
one that ran on no CPU.

## Two fail-closed surfaces this had to move with it

**`benchmarks/schemas/certification-evidence.json`** declares
`additionalProperties: false` over `importCallTiming`, so serializing two new
fields would have failed the ladder harness outright. The fields are added as
**optional**: required would reject every bundle recorded before this change,
including the S18-S24 evidence the harness must still read.

**`lifecycle_runtime.py`** seeded its accumulator with a fixed three keys and
then did `total[key] += value` over whatever the receipt carried, which raises
`KeyError` on any field added after it was written. Now `total.get(key, 0) + value`,
which tolerates the next one too.

It also surfaces `construction_call_effective_cores` per operation in the rung
summary -- omitted when CPU was unavailable and absent entirely for historical
bundles, matching how `lifecycle_application_io` already handles evidence
recorded before its instrumentation existed.

Verified: `graphforge-api` lib 745 passed / 0 failed, including a new end-to-end
test asserting a real import reports CPU for begin/append/seal and reports
`None` for the publish it never ran; benchmark harness 97 passed via CI's own
invocation; `cargo clippy --workspace -- -D warnings` exit 0; fmt clean;
`make pre-push-fast` passed.

Refs #1462

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 356d6992-2b0c-41e3-aeaa-93cdfb8cbf90

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Warning

Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the core Core source code changes label Sep 18, 2026
…1462)

`sanitized_import_operation_timings` requires exactly three fields per timed
operation:

    if timing.len() != 3 { return false; }

The two CPU fields added in the previous commit make it five, so the certify
runner rejected the receipt as `evidence_invalid` and **the S18 rung failed at
`staging_failed`**. The schema and harness changes were not enough: this is a
third fail-closed surface over the same shape, in Rust rather than JSON, and it
is the one that fails a ladder run rather than a test.

Found by running the rung. Nothing in the crate suites, the harness suites, the
clippy gate or `make pre-push-fast` catches it, because none of them execute
`certify` against a real receipt.

Accepts three fields or five, for the same reason the JSON schema makes them
optional: rejecting the old shape invalidates every bundle recorded before the
change, and rejecting the new one fails every rung after it. When present, the
CPU fields are validated -- unmeasured calls cannot exceed calls, and a zero-call
operation must carry zero of both.

Verified by re-running the rung: S18 passes, all ten phases, and the receipt
carries per-operation CPU:

    seal    calls=1   wall=26.66s cpu=24.16s eff=0.91
    append  calls=68  wall= 4.19s cpu= 3.66s eff=0.87

Refs #1462

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DecisionNerd
DecisionNerd merged commit 544fdf2 into observe/1462-effective-cores Sep 18, 2026
4 checks passed
@DecisionNerd
DecisionNerd deleted the observe/1462-wire-ingest-phases branch October 1, 2026 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core source code changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant