Skip to content

Fix Poetry/PDM scans re-parsing the lock (#1473, #1624) - #1684

Merged
Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-pypi-lock-reparse
Oct 23, 2026
Merged

Mikola Lysenko (mikolalysenko) merged 4 commits into
mainfrom
agent/fix-pypi-lock-reparse

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Oct 23, 2026 •

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-opus-5-5

Fixes #1473
Fixes #1624

Summary

Hosted Poetry and PDM scans now parse each lock text once instead of five or six times. Output is byte-identical. On the bench fixtures (400 packages, 12 patched), callgrind shows:

scenario main (aa69125) this PR Δ Ir whole-lock parses
pdm/hosted 378.4M 248.8M −34% 6 → 2
poetry/hosted 371.6M 254.6M −31% 5 → 2

Before this PR, toml_edit::Document::parse was 43–45% of all instructions in both scenarios. After it, the parse share is 22–27%.

Root cause (shared by both issues)

The Poetry and PDM hosted paths parse the same lock from several readers that knew nothing of each other. Callgrind callers of Document::parse on main:

reader text pdm poetry
lock inventory (inventory_{poetry,pdm}_lock) original 1 1
VEX discovery before the rewrite (hosted::engine::rewrite) original 1 1
PDM redirect presence probe (lock_contains, via LockParse) original 1 –
lock_fragments::rewrite_batch: the original original 1 1
lock_fragments::rewrite_batch: the rendering rewritten 1 1
VEX discovery after the write (recorded_discovery) rewritten 1 1

Only two distinct texts exist: the lock as read, and as rewritten. rewrite_batch already parsed the rewritten text (to pair fragments) before VEX parsed it again.

Fix

  • utils::lock_fragments::parse_lock is a read-only Document<String> parse behind a two-slot, bytes-keyed ParseMemo. It uses the same byte-equality contract as the vendor memos: a stale slot only ever costs a parse.
  • The lock inventory, VEX extract_poetry/extract_pdm, LockParse::parsed (the PDM presence probe), rewrite_batch's original, and the Poetry stale-install check all read through it.
  • rewrite_batch seeds the memo with the parse of its own output, so the post-write VEX read is free.
  • The step-by-step fallback (LockParse::take) copies a shared parse when one exists. Otherwise it parses privately: its intermediate texts are never read again and must not evict the two that are.
  • VEX toml_or_diag's diagnostic formatting is factored into toml_result_or_diag, so the shared parse produces the same lockfile_unparseable text.
  • Under cfg(test) the memo is thread-local, so PARSES counts stay exact across parallel tests.

No wrapper (npm/, pypi/, gem/) changes: this is core-internal.

Tests (red → green)

Issue Test Without the fix With the fix
#1473 vex::discover::pypi_locks::tests::poetry_scan_parses_each_lock_text_once (Poetry 1.2.2 / 1.8.5 / 2.1.4) 5 parses (fails) 2
#1624 vex::discover::pypi_locks::tests::pdm_scan_parses_each_lock_text_once (PDM 2.11.2 / 2.17.3 / 2.29.2) 5 parses (fails) 2

Each test runs the hosted sequence: inventory, then VEX, then batch rewrite, then VEX on the written lock. It asserts the refs found and the rewritten text, and the PARSES count. To get the red run, the memo was bypassed (memo.invalidate() before every parse_lock): left: 5, right: 2 for both. The existing many_patches_render_the_lock_once tests (one render, two parses for Poetry) still pass.

Commands run locally:

  • cargo fmt --all -- --check ✅
  • cargo clippy --workspace --all-features -- -D warnings ✅
  • cargo test --workspace --all-features --no-fail-fast: 7,522 CLI and 6,681 core tests pass. 10 fail, and all 10 are environmental in this sandbox (root user, no egress):
    • 9 chmod 0o555 write-failure tests. The 4 in core fail identically on unmodified main.
    • e2e_bun_lockb::binary_shared_bundled_record_hosted_pin_is_managed needs patches-api.socket.dev.
  • e2e_vex_build -- poetry:: --ignored with Poetry 2.4.3 ✅ (4/4)
  • e2e_vex_build -- pdm:: --ignored with PDM 2.29.2 ✅ (2/2)
  • in_process_redirect_poetry, in_process_redirect_pdm, hosted_superseding_pypi, e2e_pypi ✅
  • Bench fixtures, base vs head binary: the --json output and every written project file (sha256) are identical.

/code-review high findings not fixed

The review found no correctness bugs. Fixed: step-path memo pollution and copies (562f0ab), the Poetry stale-install check's own parse, and an overclaiming doc comment. Not fixed:

  • Retention. The memo holds up to two lock documents until the process exits. This is bounded, and it matches the vendor memos. Peak RSS during a scan is unchanged, because those documents were live during the rewrite anyway.
  • Multi-lock workspaces. A project with several poetry.lock files, or with both poetry.lock and pdm.lock, can evict the root lock's slots. A miss costs exactly what main pays today.
  • Key copy. The key duplicates the text bytes, which is inherent to ParseMemo's API. Keying on doc.raw() is a possible follow-up for every memo.
  • Other sites. The upstream-redirect and vendor-backend parses of these locks are not on the hosted-scan path that the bench measures (callgrind shows no other lock parse). They are left for a follow-up.

Follow-ups (not in this PR)

What remains in rewrite_batch is per-dep linear scans: pdm_lock_fragments_in is called 24× (6% Ir) and plan_pdm_rewrite 12× (3%), each walking every [[package]]. Indexing packages by canonical name once per batch would remove most of that.

https://claude.ai/code/session_01M3qyYKFZLy31MhprAsdm24


Note

Medium Risk
Touches shared parsing for lock inventory, VEX discovery, and redirect rewrites; behavior should be equivalent but incorrect memo invalidation could theoretically serve stale parses (mitigated by byte-equality keys and bounded retention).

Overview
Hosted Poetry and PDM scans were parsing the same poetry.lock / pdm.lock bytes repeatedly (inventory, VEX discovery, redirect probes, batch rewrite). This PR routes those read-only parses through parse_lock, a two-slot byte-keyed ParseMemo in lock_fragments, so each distinct lock text is parsed at most once per run (original + rewritten after batch rewrite seeds the memo via share_lock).

LockParse and rewrite_batch use shared Arc documents; mutable rewrite paths still clone when needed. Lock inventory, VEX extract_poetry/extract_pdm, and Poetry’s stale-install pre_1_4_writer probe go through the same entry point; VEX adds toml_result_or_diag so shared-parse failures keep the same diagnostics. Tests assert the hosted scan does two whole-lock parses per text (fixes #1473 / #1624); output stays byte-identical.

Reviewed by Cursor Bugbot for commit 562f0ab. Configure here.


Generated by Claude Code

Assisted-by: Claude Code:claude-opus-5-5
A hosted scan of a Poetry or PDM project parsed the whole lock five
or six times: the lock inventory, VEX discovery before the rewrite,
the PDM redirect's presence probe and the batch rewrite all parsed
the same original bytes, and VEX discovery parsed the written lock
again right after the rewrite had parsed that exact text. On the
400-package bench locks each parse is about a tenth of the scan.

Those readers now share one read-only parse per lock text, kept in
a two-slot bytes-keyed memo (the lock as read and as written). The
rewrite seeds it with the parse of its own output, so reading the
written lock back is free. Output is byte-identical; callgrind on
the bench fixtures drops pdm/hosted from 378M to 249M instructions
(-34%) and poetry/hosted from 372M to 255M (-31%).

Fixes #1473, #1624

Assisted-by: Claude Code:claude-opus-5-5
The one-dep-at-a-time rewrite hands each dep's output parse straight
to the next dep, which mutates it in place. Sharing that parse with
the lock memo made every later step copy it first. Only the batch
rewrite, whose output is read back by VEX discovery, shares it now.

Assisted-by: Claude Code:claude-opus-5-5
The one-dep-at-a-time rewrite parsed each intermediate lock through
the shared memo, which pushed out the original and written locks
that VEX discovery reads back, and then copied every parse because
the memo held it too. Those texts are parsed privately again; the
Poetry stale-install check reads the shared parse instead of its
own.

Assisted-by: Claude Code:claude-opus-5-5
@mikolalysenko
Mikola Lysenko (mikolalysenko) marked this pull request as ready for review October 23, 2026 08:02
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

BugBot review


Generated by Claude Code

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 562f0ab. Configure here.

@mikolalysenko Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label Oct 23, 2026
@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Ready for review (burn-down agent).

  • Head: 562f0ab
  • CI: all checks on this head pass (35 success, 50 skipped, 1 neutral, no failures).
  • Bugbot: reviewed 562f0ab, no findings. No open review threads.
  • Mergeable, no conflicts; CHANGELOG.md untouched.
  • Worth a look: the shared lock-fragment helper in utils/lock_fragments.rs, which every Poetry/PDM caller (redirect, VEX discovery, lock inventory) now uses instead of re-parsing the whole lock.

Slack announcement not sent: this run had no Slack send tool.


Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Labeled Ready for review (burn-down agent).

  • Head: 562f0ab
  • CI: 86/86 settled (35 success, 50 skipped by path filters); the one neutral row is a cancelled duplicate Bugbot run, no failures
  • Bugbot: reviewed 562f0ab, no issues; no open review threads
  • Reviewer focus: the shared lock parse in utils/lock_fragments.rs now feeds poetry/pdm rewrite, VEX discovery and the lock inventory, so check each caller still sees the same lock bytes it used to read.

Generated by Claude Code

@mikolalysenko

Copy link
Copy Markdown
Collaborator Author

Final review brief

What it does. Hosted Poetry/PDM scans used to parse the same lock 5–6 times (lock inventory, VEX discovery before and after the rewrite, the PDM probe, rewrite_batch). This adds lock_fragments::parse_lock, a 2-slot byte-keyed ParseMemo (the same memo the vendor backends already use), routes those readers through it, and seeds it with rewrite_batch's own parse of its output. Each distinct lock text is now parsed once: about 31–34% fewer instructions on the 400-package bench, with byte-identical output.

Risk: low–medium. It's a perf-only refactor on a shared parse path. A cache hit needs an exact byte match on the key, so a stale slot can only cost a parse, not give a wrong answer. Under cfg(test) the memo is thread-local, which keeps the parse counts exact. The main thing to check is that every newly shared reader is read-only, and that mutating callers clone the document (take, rewrite_batch).

Look here

  • lock_fragments.rs:50–85:`` the memo, parse_lock and `share_lock`
  • lock_fragments.rs:120–140:``` LockParse::take copies a shared parse, and a text nobody has parsed yet is parsed privately so the intermediate step texts don't evict the two that matter
  • lock_fragments.rs:541–543:```` the output parse is stored only after result == rendered has been checked above
  • pypi_locks.rs:801 and :940:````` VEX reads through the memo, and toml_result_or_diag keeps the same `lockfile_unparseable` diagnostic
  • pypi_locks.rs:2050–2140:`````` the new parse-count tests across 3 Poetry and 3 PDM versions (the description shows them red at 5 parses without the memo)

Verified

Changes I made: none.

Open questions: none blocking. The retention and multi-lock trade-offs are listed in the description; a miss costs exactly what main pays today.

Auto-merge is armed, so approving sends this straight to the merge queue.


Generated by Claude Code

Merged via the queue into main with commit ee3898c Oct 23, 2026
86 checks passed
@mikolalysenko
Mikola Lysenko (mikolalysenko) deleted the agent/fix-pypi-lock-reparse branch October 23, 2026 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

3 participants