Skip to content

perf(sessions): skip Codex scope resolution for empty pages - #4807

Closed
ScriptedAlchemy wants to merge 2 commits into
masterfrom
fix/codex-empty-reoffer-4374
Closed

ScriptedAlchemy wants to merge 2 commits into
masterfrom
fix/codex-empty-reoffer-4374

Conversation

@ScriptedAlchemy

Copy link
Copy Markdown
Owner

Scope — Refs #4374

Partial, issue-specific removal of unnecessary unchanged-rollout work. This is not a claim that the original 2,010-rollout / 28 ms-per-file incident is fully resolved; keep #4374 open until comparable measurements and all acceptance are proven on master.

Root cause and source fix

admit_jsonl_observations initializes provider state even after the authoritative source cursor and validated JSONL scan have produced an empty native-frame page. Codex's initializer then dispatched a blocking task, constructed project/profile matchers and resolved/revalidated the session cwd against Git, despite having no frame that could consume the result. Cold caches can invoke Git identity discovery; warm caches still pay task dispatch and topology revalidation.

CodexObservationAdmission::resolve_scope now returns frame-local empty verdict state before cloning roots or dispatching that work when the scan has no native frames. It does not return early from canonical admission: cursor validation, skipped-range coverage, pending source-rewrite completion and cancellation stay on the existing path. No negative scope verdict is cached, so later appended frames still resolve live scope. A dedicated sessions.observation.codex.resolve_scope tracing span supports TRACEDECAY_SPAN_TIMINGS=1 comparison.

Behavioral and performance regressions

Two new tests in the existing sessions unit-test target use real Git repositories and native JSONL files through the production Codex/JSONL admission path, with the existing MemoryHostAdmission storage fixture:

  • Seed 16 distinct rollouts/cwds, split between project and profile ownership; require nonzero accepted observations and live resolver calls. Drop the matcher cache, then exercise 96 cold/warm re-offers. Require zero Git identity resolutions, zero new captures/decoded frames/consumed source bytes, unchanged committed source identities/cursors, and full retained coverage. Record actual elapsed time and resolution work counts. Restoring the old initializer makes the zero-resolution assertion fail.
  • After an empty re-offer, append a real cwd transition out of the project and back. Require new records to reach only the correct scope with the original canonical source identity, advanced coverage, and subsequent no-op convergence. Pre-cancellation must remain deferred, not successful completion.

These are new regression assertions, not executed benchmark results. The memory fixture does not establish SQLite restart/durability or affected-host end-to-end performance. Existing metadata-fill, same-size rewrite, pending-rewrite and discovery-restart tests remain unchanged and must also pass.

Verification and merge hold

Published commit: 408fdb7ec143e31e6ced01b75a2bcff1eca847ed, one parent at master 91c72aba7750a398dd28e83d64bf0c6d6fef4172. GitHub compare confirms exactly two files, 20 production/module-wiring lines plus the focused regression module. Published via an exact expected-old-head nonforce fast-forward. No existing branch was overwritten. The generated Bazel source glob already includes the test module.

The authoring environment has no supplied checkout or Bazel executable, and cannot resolve GitHub from its shell; no local Rust/Bazel pass or measured speedup is claimed. This ordinary non-model PR is ready for review to allow normal hosted Bazel CI, not to waive validation. Require nonzero execution of:

bazel test //crates/tracedecay-sessions:unit_test --test_output=all --test_arg=reoffer_tests
bazel test //crates/tracedecay-sessions:unit_test --test_output=errors
bazel build //crates/tracedecay-sessions:rustfmt_check_2024

Inspect exact-head CI, native macOS qualification and independent review before merging. Do not treat a queued or skipped job as a pass. Cargo was not used; CI and budgets were not changed.

Coordination / remaining acceptance

No file overlap with #4756's current #4793 projection-queue work. No code was copied from held mixed DRAFT #4776, and no Kev/decision-model work was included, moved, merged or auto-merged. Those PRs remain under their draft/merge hold. Release #4763, production, infrastructure, tags, credentials and other task schedules are untouched.

Still required: comparable full affected-host before/after project/profile traces for the original corpus, attribution of remaining cursor/store and metadata/provenance costs, exact-head Bazel/native execution and master-proven acceptance. Refs #4374 only; do not close the issue on publication or on this partial fix alone.

Refs #4374

Keep canonical cursor validation, source rewrite completion and cancellation
on the admission path, but do not dispatch Git scope resolution when no
native frame can consume its result. Do not retain an empty-page verdict.

Add real JSONL/project/profile regressions for cold and warm re-offers,
resolution work counts, unchanged accepted identities, later cwd changes,
appended records and cancellation. Add a dedicated scope-resolution span.

Bazel execution is pending hosted CI; no local Rust pass is claimed.
@changeset-bot

changeset-bot Bot commented Nov 8, 2026 •

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: 95d7bcf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Apply the five exact Bazel rustfmt diagnostics to the focused
Codex reoffer regressions without changing tested behavior.

Refs #4374

Copy link
Copy Markdown
Owner Author

Superseded by #4806, now with this PR's exact formatted reoffer_tests.rs blob (d6272604004af8dcb46364b8540bb8753432c207) and the dedicated scope-resolution timer, published as commits 3386b3c and 08b01b9. The existing #4806 Codex-only fast path preserves rewrite-completion and non-opted-in provider validation. The 16-rollout/96-reoffer tests here now run through that one review surface. Closing this duplicate lane without deleting or rewriting its branch; neither PR has a passing exact-head Bazel/native test or measured affected-host speedup yet. Do not merge this duplicate PR. Refs #4374; issue remains open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant