Skip to content

fix(pipeline): bound cross-file preparation memory - #2909

Open
chang-chaunce wants to merge 1 commit into
DeusData:mainfrom
chang-chaunce:codex/cross-lsp-memory-budget
Open

chang-chaunce wants to merge 1 commit into
DeusData:mainfrom
chang-chaunce:codex/cross-lsp-memory-budget

Conversation

@chang-chaunce

Copy link
Copy Markdown

What does this PR do?

Cross-file LSP preparation can allocate far beyond the worker's memory budget after extraction has already spilled its results. In an observed macOS arm64 run on 0.12.1, charged footprint reached 19,169 MiB against a 7,090 MiB budget during this phase.

Two allocations contribute to this: C# and JVM partition scratch arrays use the definition count of the entire project, including unrelated JSON definitions, and the global definition collector allocates its array and copied strings without a budget refusal path.

This change:

  • Sizes C# and Java/Kotlin scratch arrays by their matching language definitions, preserving the stable type-first partition.
  • Checks the remaining charged-memory budget before allocating the global array and periodically while collecting definitions; checks between parallel registry builders.
  • Propagates a distinct budget failure through parallel, sequential and incremental callers, cleans temporary state, and preserves the serving database instead of treating the refusal as a soft LSP failure.

Related context: #1997 and #2711. No configuration defaults, MCP tool APIs, dependencies or build-system changes.

Validation

  • scripts/test.sh --suites 'mem pipeline cs_lsp java_lsp': 689 passed, ASan + UBSan, macOS arm64; build parallelism capped at two.
  • Reproduce-first control: reverting only the two registry scratch-array fixes makes the new regression fail (arena capacity 22,060,048 versus 458,752 bytes with the same matching C# definitions). Restoring the fixes and rebuilding: all 4 new regression tests pass.
  • The regressions also cover a metadata-only oversized collector array refusing before dereference, and a real pipeline budget refusal leaving the previous database byte-for-byte unchanged with no staged-generation artifacts.
  • Whole-tree clang-format 20.1.8, no-skips, NOLINT whitelist, memory-core ratchet, and git diff --check pass.
  • scripts/lint.sh --ci was attempted with local cppcheck 2.22.0 (CI pins 2.20.0). It reports existing findings in unchanged code. In the two touched pipeline files, a separate check of the unmodified base reproduces the same four control-flow findings as the patched files; no additional finding remains. The pinned 2.20.0/full default test leg has not been run locally.

The memory checks are cooperative and do not replace the optional process-tree watchdog. The original large private corpus was not rerun; the committed regressions are self-contained and need no private data.

Checklist

  • Every contribution commit carries a matching DCO sign-off.
  • Focused sanitizer tests and reproduce-first regression pass locally.
  • New behavior is covered by self-contained tests.
  • Formatting and memory-allocation policy checks pass.
  • Full upstream CI / pinned cppcheck 2.20.0 validation.

AI-assisted contribution: prepared with OpenAI Codex on behalf of @chang-chaunce.

Size C# and JVM scratch arrays by matching definitions. Refuse global cross-file allocations over the remaining budget and propagate budget aborts without replacing the serving index. Add self-contained allocation and publication-preservation regressions.

Signed-off-by: chaunce <chang.chaunce@gmail.com>
@github-actions

github-actions Bot commented Nov 4, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@DeusData

DeusData commented Nov 4, 2026

Copy link
Copy Markdown
Owner

Hi @chang-chaunce, thank you for this careful PR. The reproduce-first control, the arena-capacity test instead of an RSS guess, and the byte-for-byte check that the serving database survives a refusal are exactly how we like memory work to arrive.

We measured it before deciding: the Linux kernel, Elasticsearch, and Exposed with a generated 3-million-key JSON archive. The graph is byte-identical to main on all of them, and your new tests fail without the fix for the right reasons. One part we would like to change:

  1. Please keep the C# and Java/Kotlin scratch sizing, cbm_mem_would_exceed_budget(), the up-front refusal before the project-wide calloc, the error propagation, and all four tests as they are.
  2. Please remove the instantaneous checks: the per-file and every-256-definitions checks in cbm_pxc_collect_all_defs, and the checks before and after each shared registry builder in run_parallel_pipeline. With a 4500 MB budget on the JSON-archive repository, main finishes about 9% over budget, while this branch fails the whole index at the first builder check. In this codebase the budget is meant to stop a run only as a last resort, after relief such as spilling and back-pressure has been tried and the run is still over (see the comment above cbm_mem_should_relieve() in src/foundation/mem.c). A check with no relief step in front of it turns a small, short overshoot into a failed index. None of your tests depend on these checks, so they should stay green.
  3. Optional nit: the test variable "small" can collide with a Windows header macro (rpcndr.h defines it); a name like "matching_only" avoids that.

If you would rather we make these edits on your branch, just say so; maintainer edits are enabled and we are happy to. Thanks again!

This branch has not been deployed

No deployments
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.

2 participants