Repository navigation
observe(api,bench): carry process CPU on every construction operation (#1462) - #1474
Merged
Merged
Conversation
…#1462) (#1471) * observe(api,bench): carry process CPU on every construction operation (#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> * fix(bench): let certify accept the CPU fields, or every rung fails (#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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: CurateLabs/graphforge/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
… surface
The public method inventory goes from 387 to 388. The delta was computed
against `main` rather than inferred from the digest mismatch, and it is
exactly one method added and none removed:
ImportCallTiming.effective_cores
`ImportCallTiming` takes the `introspection` receiver default -- "value-object
constructor or accessor classified outside persisted facade behavior" -- which
is what this is: it divides measured process CPU by elapsed wall for one
construction call and returns `None` when CPU was not measured, rather than
guessing. It is not a persisted facade contract.
The pinned `public_method_digest` and the inventory count in the gate's own
test move with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
testing
`EXPECTED_RUST_DIGEST` in the Python binding's release tests pins the same value as `public_method_digest` in the surface manifest, and the parity policy asserts the two agree. Classifying `ImportCallTiming.effective_cores` moved one and not the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
DecisionNerd
added a commit
that referenced
this pull request
Sep 18, 2026
Resolves the public-surface digest collision with #1474. Both branches added one public method and independently moved the pinned inventory from 387 to 388: #1474 ImportCallTiming.effective_cores #1415 StorageAllocationDiagnostics.peak_detail Together they are 389, so neither side's digest is correct on the merge. The combined digest was derived from the union of the two method sets and then checked against the gate rather than pasted; the same derivation reproduces each branch's own digest exactly, which is what makes it trustworthy. Note the third pinned location, the inventory count in `test-non-cypher-surface-gate.py`, did **not** conflict: both sides made the identical 387 -> 388 edit, so git merged it silently to a value that is wrong for the merge. Only the two digests conflicted. A count that agrees on both sides and is wrong on neither is still wrong on the union. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 18, 2026
observe(storage): nothing measures the serial fraction of ingest, and #1387 budgets against it
#1462
Closed
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
1 participant
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.
Re-targets #1471 onto
main.#1471 was opened against
observe/1462-effective-cores(the #1470 stack). #1470 landed onmainsquashed, which left #1471's base branch merged but stale; merging #1471 then put it on that dead branch rather than onmain. This is the same commit, cherry-picked onto currentmain, with no content change.ImportCallTiminggainscpu_nsandcpu_unmeasured_calls, and the fourInstant::now()sites becomeCallStart::now().Two compatibility points are load-bearing and are preserved here:
sanitized_import_operation_timingsaccepts a 3- or 5-field timing tuple. Requiring 5 would fail every ladder rung withevidence_invalid.cpu_ns/cpu_unmeasured_callsschema fields are optional, because the evidence schema isadditionalProperties: falseand historical bundles do not carry them.Closes #1462.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.