BUILD: Take the reload leak fix into the engine - #343
Merged
Conversation
A failed data file reload leaked the replacement data set and its file pool handles, which from data file format 4.6 is the steady state for every deployment still on a 4.5 engine with automatic update enabled, because only 4.6 files are produced and every update attempt fails the version check inside the reload. Fixed in common-cxx by freeing the replacement through the engine's free method on the failure path (51Degrees/common-cxx#149, merged), proven by a C++ regression test that fails on the previous pin with the leak assertion and the held file handle, and passes on this one (51Degrees/ip-intelligence-cxx#157). The submodule moves to the head of that pull request, which carries the merged common-cxx fix, and repoints at its merge commit when it lands. No managed change: the reload functions gained a parameter the FIFTYONE_DEGREES_DATASET_RELOAD macro supplies, so the engine recompiles unchanged. Analysis with line references: 51Degrees/ip-intelligence-cxx#156
jwrosewell
added
type: bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A failed data file reload leaks the replacement data set and the file pool's open handles (ip-intelligence-cxx#156). From data file format 4.6 that failure is the steady state for every deployment still on a 4.5 engine with automatic update enabled, because only 4.6 files are produced from 29 August and every update attempt fails the version check inside the reload, leaking a set of handles per attempt. On this engine an in-memory configuration leaks the whole in-memory copy per failed reload, 7.7 GB on the current Enterprise file. James Rosewell asked on 31 August for the fix to be forced through and the download stream packages re-released before the cloud release.
What changed
FiftyOne.IpIntelligence.Engine.OnPremise/ip-intelligence-cxxmoves fromcd288870to0c3e759e, the head of ip-intelligence-cxx#157, which carries two things:src/common-cxxatae6d4c76, the merge commit of common-cxx#149. Both reload functions indataset.ctake the engine's free method and call it on the init failure path, and theFIFTYONE_DEGREES_DATASET_RELOADmacro passes each engine's ownfreeDataSet, so this engine recompiles unchanged and no managed or SWIG change is needed.ReloadFailure, in both the file and memory engine suites (14 instances across the configuration matrix). Verified on Windows x64 Debug in both directions: on the previous pin the test fails with the suite's leak assertion and, in the file variant, with the invalid file still held open; on this pin all 14 pass and the existing reload success test still passes.The pin repoints at #157's merge commit when it lands; the content is identical.
Verification
The C++ proof above is local to Windows. The Publish run for this package builds the native code on Windows, Linux and macOS and runs the full .NET suites against the 4.6 Enterprise file before packaging, which is the cross-platform compile and regression evidence, and the ip-intelligence-cxx Pull Requests workflow can add the C++ matrix when dispatched. The dispatch of Publish for this package should not skip tests.
Produced with AI assistance under James Rosewell's direction and needs human review.