Skip to content

fix(windows): keep aligned allocations on mimalloc - #2260

Open
knewstimek wants to merge 2 commits into
DeusData:mainfrom
knewstimek:fix/windows-aligned-mimalloc
Open

knewstimek wants to merge 2 commits into
DeusData:mainfrom
knewstimek:fix/windows-aligned-mimalloc

Conversation

@knewstimek

Copy link
Copy Markdown

Windows production builds can crash in _aligned_free while cleaning up parallel indexing workers.

When CBM_MEM_GLOBAL_OVERRIDE is enabled, use mi_malloc_aligned and mi_free directly. Builds without the override keep using the CRT allocator.

Tested with a Windows production build and a 64-file synthetic Go fixture at 2 and 4 workers.

Signed-off-by: News <knewstimek@users.noreply.github.com>
@DeusData

Copy link
Copy Markdown
Owner

Thank you — a production crash on Windows in code that shipped two days ago is exactly what I want to hear about fast, and a nine-line fix with a clear description is the best possible way to hear it.

The principle is right, and it is the one this codebase already tries to hold everywhere else: an allocation and its release must live on the same allocator. Under CBM_MEM_GLOBAL_OVERRIDE the process runs on mimalloc, and pairing CRT _aligned_malloc with CRT _aligned_free there only works if both halves resolve to the same runtime. Going to mi_malloc_aligned / mi_free directly removes the question instead of answering it.

My working hypothesis for why it crashes — offered as a hypothesis, not a finding, since I cannot reproduce on Windows today: on MinGW the aligned pair can come from two different places. _aligned_malloc from the static libmingwex calls malloc, which under our --wrap link resolves to the mimalloc-backed wrapper; _aligned_free imported from ucrtbase.dll calls the CRT's own free on a pointer mimalloc owns. That would crash in _aligned_free, during cleanup, only in production builds — which is your report exactly. If you happened to see which module the faulting _aligned_free frame was in, that would confirm or kill it.

One thing about verification, so you know what I am waiting on and why. CBM_MEM_GLOBAL_OVERRIDE is defined only in CFLAGS_PROD, so none of the test-* legs compile your new branch — a green test matrix says nothing about this change. The check that matters is pr-smoke on windows-latest, which builds the production binary and indexes a fixture with it. I will merge on that, together with your own 2- and 4-worker run, rather than on the matrix.

Two small questions, neither blocking:

  1. Was your crash on x64 or arm64? Our local Windows venue is arm64 and hosted CI is x64, so it tells me which of the two would have caught it.
  2. Did 64 files at 2 and 4 workers reproduce it reliably before the fix, or intermittently? If intermittent, I would like to turn your fixture into a regression guard that loops the worker teardown, because a crash that needs luck to appear needs a test that does not.

@knewstimek

Copy link
Copy Markdown
Author

x64.

The original crash was on a larger repository: 2 and 4 workers crashed in _aligned_free, while 1 worker completed. The 64-file synthetic fixture was added as post-fix coverage, so I have not confirmed that it reproduces the crash reliably before the fix.

@DeusData

Copy link
Copy Markdown
Owner

Thank you — that is exactly the detail I needed, and quickly.

x64 settles the venue question: our local Windows machine is arm64, so hosted CI's pr-smoke on windows-latest (x64, production flags) is the only leg of ours that builds the code path you changed. That is the check I am waiting for; the rest of the matrix does not compile this branch, so I will not read anything into it either way.

And thank you for being straightforward that the 64-file fixture was added after the fix and has not been shown to fail before it. That is useful to know rather than a problem: it means the fixture is coverage, not yet a regression guard, and it does not hold up this PR. "1 worker completes, 2 and 4 crash in _aligned_free" on a larger repository is a clear enough signature on its own — a release that only goes wrong when more than one worker tears down is what an allocator mismatch looks like.

What I will do on our side after this lands is try to turn it into a guard that fails deterministically without the fix — most likely by running the worker teardown against a production-flag Windows build rather than by making the fixture bigger, since size only raises the odds. If that works I will credit you on it. Nothing further is needed from you for this PR.

@github-actions

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.

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