fix(windows): enforce index worker memory budgets by experticket-admin · Pull Request #2030 · DeusData/codebase-memory-mcp

experticket-admin · 2026-09-03T16:32:35Z

What does this PR do?

Adds a Windows OS-enforced commit backstop for an index worker and its descendants, using JOB_OBJECT_LIMIT_JOB_MEMORY on the existing Job Object. Related: #832; complements the cooperative over-budget abort in #2112.

Updated after maintainer review:

Keep the supervisor's internal worker budget unchanged in argv.
Below 512 MiB (including zero and the 4096/8192-byte test budgets), disable the OS memory cap. Process-tree containment and KILL_ON_JOB_CLOSE remain enabled.
For budgets of at least 512 MiB, use a separate commit cap of budget + budget / 2, with overflow saturation. A 3 GiB internal budget therefore gets a 4.5 GiB Job Object cap.
Configure the Job Object before creating the suspended worker; assign it before resuming. Existing fail-closed launch behavior remains.
Capture terminal Job Object limit/peak diagnostics and log them for unsuccessful capped workers, without inferring OOM or changing file quarantine/classification.
Initialize the new result fields even on an early rejected launch, preventing stale diagnostics on result reuse.

Scope and limitations

Windows commit is not RSS. The headroom allows room for cooperative recovery/abort, but does not guarantee that it will run before an allocation is denied. This is a per-job backstop, not a global host-memory cap: existing active jobs are not retroactively resized when the daemon starts another job. Zero-budget legacy callers remain uncapped. Other subprocess callers default to no memory cap; this does not impose a general limit on Git or search tools.

This does not fix indexing/allocator growth, guarantee that a repository will fit, or establish the cause of a whole-PC freeze. Automatic reindexing is unchanged.

Regression coverage

Policy boundaries: zero, tiny budgets, 512 MiB boundary, realistic budgets, overflow.
Native Windows positive/negative control: the same 2 GiB VirtualAlloc must succeed without a cap and return the probe's allocation-denied exit code with a 1 GiB cap. The probe does not touch the allocated pages and both process trees must be quiescent.
Terminal limit/peak reporting through supervisor logging; no memory warning for a successful worker.
Result reuse after rejected launch clears all newly added fields.

Why the test does not assert peak <= cap: an independent Windows API probe showed a denied 2 GiB reservation reflected in PeakJobMemoryUsed even with a 1 GiB cap. The uncapped/capped allocation outcomes prove denial; the peak counter alone does not.

Local verification (2026-09-11)

Native Windows x64, MinGW GCC 16.2: subprocess 16 passed, 18 platform skips, four consecutive direct executions. Earlier cold/MSYS-launched runs had startup/quiet-timeout failures and are not counted as successful runs.
Native index_supervisor: policy and terminal-memory-log tests pass, but the suite aborts later in an existing progress-capture test with 0xC0000374 at fclose(tmpfile()). A small program with no subprocess/supervisor code reproduces the same failure when linked with unchanged main's Windows mimalloc wrappers; without those wrappers it exits normally. This does not certify the whole Windows suite or establish that CI's different Clang/UCRT toolchain reproduces it.
Changed production files: canonical scripts/lint.sh --ci with scoped inputs passes (clang-format 20.1.8, cppcheck 2.13; CI uses cppcheck 2.20). No-skips policy, no-window contract, DCO and git diff --check pass. This is not a claim that repository-wide lint/tests passed.
Linux GCC 13.3 ASan+UBSan, canonical scripts/test.sh --suites "subprocess index_supervisor": 41 passed, 2 platform skips, exit 0. The progress-capture test that aborts in the local Windows/MSVCRT build passes here. The isolated Linux image needed zlib1g-dev after the main update; no host dependency change was made.
External text-only review found no blocking findings; runtime validation limitations above remain.

The native local build requires two command-line warning-severity exceptions for pre-existing GCC 16 warnings in unrelated tests (free-nonheap-object, stringop-overflow). No suppressions were added to the repository. No test build has been installed on the affected PC.

Checklist

All new non-merge commits carry DCO sign-off.
New behavior has regression coverage.
Full test suite passes locally (focused results and blockers documented above).
Repository-wide CI/lint passes (not claimed from scoped checks).

Signed-off-by: Subgurim <[email protected]>

github-actions · 2026-09-03T17:25:14Z

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 · 2026-09-03T18:15:31Z

Thank you for the contribution and for framing this as an OS containment boundary rather than claiming it fixes the allocator or indexing growth itself.

This is routed as a high-priority Windows stability change. Job Object memory limits affect worker termination and descendant behavior, so we need more time to review the native semantics and the regression probe carefully. The contribution queue is quite full, but this has a clear operational safety claim and will receive a proper review.

No changes are requested from you at this triage stage. Thank you for including measured v0.10.8 evidence and a Windows-specific test path.

DeusData

Welcome, and thank you for this — it is a well-aimed PR at a real gap. The #832 incident numbers in your description (a 3 GiB budget beside ~40 GiB workers) are exactly the case a Job Object limit exists for, and I would like to land it. I built it on macOS and ran it on a native Windows machine; the plumbing is right, but the value it hands to the OS is not safe yet. Details below, with what I saw and what I would ask you to change.

What is right

JOB_OBJECT_LIMIT_JOB_MEMORY + JobMemoryLimit is the correctly paired flag/field, and job-wide is the right scope — descendants are covered (src/foundation/subprocess.c:664-665).
Ordering is airtight: the limit is set on the job before CreateProcessW, the worker is created suspended, assigned to the job, and only then resumed (:661-673, :741, :758, :766). No worker instruction runs outside the cap.
It extends the existing job rather than creating a second one, so the nested-job case (Windows 8+, CI runners already inside a job) is no new risk.
0 means no limit, units are bytes end-to-end (application.c:2926-2928), and the field is documented honestly as Windows-only in subprocess.h.
The test is a genuine reproduction. I reverted only the subprocess.c production hunk on the Windows box and re-ran: subprocess_windows_job_object_enforces_memory_limit then fails for the right reason (outcome == 0, expected CBM_PROC_EXIT_NONZERO — the 512 MiB VirtualAlloc succeeds and the probe exits 0). It asserts the child's observable outcome, not merely that an API returned success. scripts/check-no-test-skips.sh is clean and the test is wired into SUITE(subprocess), which the runner really executes.

The blocker — proven on real Windows

build/c/test-runner subprocess index_supervisor on Windows (ARM64) with the PR as-is:

index_supervisor_async_jobs_are_isolated_cancellable_and_terminal_cached
  outcome=crash exit_code=-1073741571        (0xC00000FD STATUS_STACK_OVERFLOW)
  [worker-dump] worker-a log: (empty)
  FAIL tests/test_index_supervisor.c:510: ASSERT(first_ready)

With the subprocess.c hunk reverted the same test passes and the totals stay 22/1/18 — the single failure just moves from the supervisor suite to your new test, which is the signature of a clean revert-check. index_supervisor runs in every CI shard, so the Windows leg will go red on this once the queue drains (it is congested today; the test-msan and test-windows-guards reds you will see on other PRs are ours, not yours).

Cause: the same number is now used twice, with no floor. src/mcp/index_supervisor.c:669-674 passes memory_budget_bytes to the worker as its internal soft budget (--memory-budget → main.c:1066 → mem.c:257-265), and :698 now passes the identical value as a hard OS commit cap. Those supervisor tests start workers with token budgets of 4096 and 8192 bytes (tests/test_index_supervisor.c:390,392 — they only assert the value reaches the child argv). Under the PR that becomes a 4 KiB / 8 KiB commit limit, and with the binary's 8 MiB stack reserve (-Wl,--stack,8388608) the worker dies before main() — hence the empty log. Anyone setting a small CBM_MEM_BUDGET_MB gets the same undiagnosable worker_failed outcome=crash.

The deeper mismatch: the budget is soft, the Job limit is hard, and they measure different things. cbm_mem_over_budget() compares RSS (mem.c:511-515); a Job Object limit is on commit charge, which is ≥ RSS. And the pipeline is designed to overshoot the budget: when backpressure naps are futile the worker latches bp_futile and proceeds with a documented soft overshoot (pass_parallel.c:760-784), and over-budget during dump only triggers opportunistic freeing (graph_buffer.c:1732). With the cap at exactly the budget, VirtualAlloc is denied mid-pipeline at whatever site is unlucky — there is no global OOM handler — so "slow, over budget, completes" becomes "index fails". The graceful path is bypassed, not assisted.

Changes I would ask for

A floor. Do not set JOB_OBJECT_LIMIT_JOB_MEMORY below a sane minimum (a few hundred MiB is a reasonable start); below it, keep the job with kill-on-close only. This alone clears the supervisor RED. Keep the arithmetic in the supervisor (policy) and leave subprocess.c as pure mechanism — the split you already have.
Headroom above the soft budget — JobMemoryLimit = budget × factor (1.25–1.5× is the range I would start from) so backpressure and dump-time freeing get to act before the OS denies commit. Put the reason in a comment next to the flag: RSS (mem.c:511) vs commit, and the deliberate overshoot path. One design point I want to settle on our side before you finalise this item: whether the OS cap should be a deliberately looser backstop (my expectation) or sit exactly at the budget with the internal budget lowered instead. I will confirm here shortly; items 1, 3 and 4 do not depend on it.
Make the failure diagnosable. When a worker fails, read PeakJobMemoryUsed (JobObjectExtendedLimitInformation) or JobObjectLimitViolationInformation and log an index.supervisor.worker_over_budget-style line with peak and limit. Today the operator gets an empty worker log and a bare exit_code=-1073741571.
(nit) Give the test probe more distance from the process baseline — e.g. a 1 GiB limit with a 2 GiB allocation. The Windows test-runner is ~393 MB against the current 512 MiB limit, so on a host where startup commit exceeded the limit the child would die with a non-73 status and the test would fail for the wrong reason.

Two small things you can ignore or fold in as you like: the probe uses GetModuleFileNameA while production self-discovery is pinned to the W variant (win_utf8.h:174) — existing tests do the same, but a non-ASCII build path would break the new one; and char self_path[32768] is a hefty stack buffer for a test.

Everything else — security, scope, format (the test_main.c:25 clang-format hit is byte-identical on main, not yours) — is clean. Once the floor and headroom are in, I expect this to be a quick merge.

DeusData · 2026-09-03T18:46:07Z

Settled on our side: the OS cap is a looser backstop, not a hard cap at the budget. The internal budget stays advisory with its documented overshoot path; the Job Object limit sits above it to catch runaways like the #832 case (13× over), and the floor keeps small or token budgets from being fatal at startup.

So item 2 is as written: JobMemoryLimit = budget × factor with the factor documented next to the flag (1.25–1.5× is a fine starting point — say why you pick what you pick), plus the floor from item 1. No change to mem.c or pass_parallel.c needed. Thanks again — looking forward to the update.

Signed-off-by: Subgurim <[email protected]>

DeusData · 2026-09-09T19:04:04Z

Hi @experticket-admin — checking in on this one, and we'd still very much like to land it. The mechanism is right: the Job Object memory limit, the set-before-CreateProcessW / suspended-assign-resume ordering, the job-wide scope covering descendants, and the reproduction test (which fails for the right reason when the production hunk is reverted) are all solid.

The one thing still blocking a merge is the point from the review above: the memory-limit value handed to the OS isn't safe yet. Once that's addressed, this should be ready — and the red Windows CI leg looks like our known flaky lane rather than anything in your change, so don't worry about that one; we can re-run it.

No rush at all — whenever you have a moment to push the value fix, drop a note here and we'll take another pass and get it merged. Thanks again for tackling a real gap (#832).

…l sin techo Signed-off-by: Subgurim <[email protected]>

Signed-off-by: Subgurim <[email protected]>

experticket-admin · 2026-09-11T11:47:36Z

Updated to eeff3f19 and merged main through 055fbb7d. The PR description now reflects the revised policy and actual validation limits.

Addressed the review requests:

Tiny/zero budgets keep their argv values but do not activate the OS memory cap below 512 MiB.
Real budgets get a separate, overflow-safe 1.5x Job Object commit cap, not an exact RSS-budget cap.
Failed/cancelled capped workers report terminal limit/peak diagnostics without inferring OOM or changing quarantine behavior.
Added policy boundary/overflow tests, supervisor log assertions and early-result-reset coverage. The reset test failed before the initialization fix and now passes.

I also corrected the native enforcement test based on a real Windows observation. An independent API-only probe gave:

Probe	Child exit	Job cap	PeakJobMemoryUsed
Same 2 GiB reservation, uncapped	0 (accepted)	0	2,155,827,200
Same reservation, capped	73 (denied)	1,073,741,824	2,155,814,912

Thus peak <= cap is not a valid assertion here: the denied reservation appears in the peak counter. The regression test now requires the same allocation to succeed uncapped and fail capped, with both trees quiescent. It does not touch the 2 GiB of pages. Memory diagnostics are deliberately evidence, not an inferred cause.

Validation

Native Windows x64, GCC 16.2/MSVCRT: subprocess 16 passed, 18 platform skips, four consecutive direct executions. Earlier cold/MSYS-launched runs hit startup/quiet timeouts; I am not treating them as passes.
Native supervisor policy and memory-log tests pass, but a later pre-existing progress-capture test aborts at fclose(progress) with 0xC0000374. I isolated this without any subprocess or supervisor code: tmpfile → fputs → fseek → fread → fclose, linked against unchanged main's Windows mimalloc wrappers, produces the same heap-corruption exit. Without the wrappers it exits 0. Even redirecting only strdup to mi_strdup reproduces it with this toolchain. This narrows the local validation blocker; it does not establish a CI Clang/UCRT failure or a whole-host freeze cause. No allocator change is included in this PR.
Scoped canonical production lint, no-skips, no-window contract, diff whitespace and DCO passed. Local cppcheck is 2.13 versus CI 2.20; clang-format is 20.1.8. Native GCC builds require command-local severity exceptions for pre-existing unrelated test warnings; no repo suppressions were introduced.
Linux GCC 13.3 ASan+UBSan, canonical scripts/test.sh --suites "subprocess index_supervisor": 41 passed, 2 platform skips, exit 0. The progress-capture test that aborts in the local Windows/MSVCRT build passes here. The isolated Linux image needed zlib1g-dev after the main update; no host dependency change was made. The current PR CI is still running; full green status is not claimed.

An external text-only review found no blocking issue in the revised change. The final commits after that review clarify/format a comment only.

This remains a per-worker-tree commit backstop. It neither guarantees aggregate host memory (already-running daemon workers are not retroactively resized), nor guarantees cooperative checks will always run before a denied allocation. No experimental binary has been installed on the affected PC.

DeusData · 2026-09-12T17:41:05Z

Merged as 9c6f45bd — thank you for coming back to this, and for taking the value-safety point seriously rather than arguing it.

The mechanism was right from the first revision: JOB_OBJECT_LIMIT_JOB_MEMORY set on the job before CreateProcessW, the process created suspended and resumed after, so there is no window where a descendant runs uncapped. That ordering is the part that is easy to get subtly wrong, and you had it airtight.

What was missing was the value, and eeff3f19 fixes exactly that:

A floor instead of a blind multiply. Below 512 MiB the OS cap is simply not set. A tiny token budget would otherwise be capped below what image, CRT and stack startup need before main is even reached — a limit that kills the worker for reasons unrelated to indexing. KILL_ON_JOB_CLOSE still applies, so descendants remain contained either way.
A real overflow guard. budget > SIZE_MAX - headroom ? SIZE_MAX : budget + headroom is the correct safe-add. This was the actual hazard: on a large budget the naive budget + budget/2 wraps and hands the OS a tiny cap, which fails in the worst possible direction — looking like a successful limit while strangling the worker.

The comment earns its place too. Saying plainly that Windows charges job-wide commit, not RSS, and that 1.5× is a backstop for descendants rather than exact enforcement of the accounting budget, is the kind of honesty that stops the next reader trusting this more than it deserves. Same for the diagnostics: reporting job_limit_bytes and peak_job_memory_bytes as evidence, with peak explicitly marked unavailable when it is, instead of inferring an OOM classification the data cannot support.

This completes the Windows half of the memory work — the cross-platform side (an over-budget index attempt failing whole with a named error, keeping the serving index) landed in #2112, and your Job Object cap is the OS-enforced backstop underneath it for the worker and everything it spawns.

It ships in v0.10.9. Thanks for your patience through the review round — the delay was ours.

fix(windows): enforce index worker memory budgets

3507d95

Signed-off-by: Subgurim <[email protected]>

experticket-admin requested a review from DeusData as a code owner September 3, 2026 16:32

experticket-admin mentioned this pull request Sep 3, 2026

Memory not released after indexing: 20GB+ RSS for 5MB of indexed data #832

Closed

DeusData added bug Something isn't working stability/performance Server crashes, OOM, hangs, high CPU/memory windows Windows-specific issues priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 3, 2026

DeusData mentioned this pull request Sep 3, 2026

Memory leak: RSS grows without bound and is not released after indexing (still present on latest release) #2004

Closed

2 tasks done

DeusData requested changes Sep 3, 2026

View reviewed changes

DeusData mentioned this pull request Sep 8, 2026

fix(pipeline,mcp,daemon): fail an over-budget index attempt whole with a named reason (#1997, #832) #2112

Merged

fix(windows): add headroom and diagnostics to worker memory limits

2b8bd0c

Signed-off-by: Subgurim <[email protected]>

subgurim added 4 commits September 11, 2026 13:25

fix(windows): inicializa diagnósticos y verifica el límite con contro…

262ab01

…l sin techo Signed-off-by: Subgurim <[email protected]>

merge: actualiza la base de PR2030 con main

9311177

Signed-off-by: Subgurim <[email protected]>

docs(windows): aclara el alcance del margen de memoria

e65bc6b

Signed-off-by: Subgurim <[email protected]>

style(windows): ajusta el formato del comentario de memoria

eeff3f1

Signed-off-by: Subgurim <[email protected]>

DeusData merged commit 9c6f45b into DeusData:main Sep 12, 2026
34 checks passed

Navigation Menu

Repository navigation

fix(windows): enforce index worker memory budgets - #2030

fix(windows): enforce index worker memory budgets#2030
DeusData merged 6 commits into
DeusData:mainfrom
experticket-admin:fix/windows-worker-job-memory-limit

experticket-admin commented Sep 3, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

DeusData commented Sep 3, 2026

Uh oh!

DeusData left a comment

Uh oh!

DeusData commented Sep 3, 2026

Uh oh!

DeusData commented Sep 9, 2026

Uh oh!

experticket-admin commented Sep 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

DeusData commented Sep 12, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

experticket-admin commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Scope and limitations

Regression coverage

Local verification (2026-09-11)

Checklist

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

DeusData commented Sep 3, 2026

Uh oh!

DeusData left a comment

Choose a reason for hiding this comment

What is right

The blocker — proven on real Windows

Changes I would ask for

Uh oh!

DeusData commented Sep 3, 2026

Uh oh!

DeusData commented Sep 9, 2026

Uh oh!

experticket-admin commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation

Uh oh!

Uh oh!

DeusData commented Sep 12, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

experticket-admin commented Sep 3, 2026 •

edited

Loading

experticket-admin commented Sep 11, 2026 •

edited

Loading