Conversation
… result doDbCleanup deleted query samples and query jobs older than the TTL, and nothing else (danielplohmann#68). It also read result["info"]["sample"] off every finished and failed query job, so the first failed job without a result (one that died before matching) took the whole cleanup down with a TypeError. The cleanup now treats a job without a result as nothing to protect - it is deleted once older than the TTL, kept otherwise - and after the TTL pass removes the query functions whose query sample is gone and the query disassembly whose function is gone, through deleteOrphanedQueryData() on the storage. With STORAGE_MONGODB_COMPACT_AFTER_CLEANUP (default off, it needs the compact privilege) it then runs MongoDB's compact on the collections the deleted query data and results lived in. The job answers a report of what it deleted and freed.
…t the compaction knob The cleanup took a client-side snapshot of the query samples and then deleted every query function outside it, so a query being inserted on another worker could lose its functions. It now takes the newest query function id first and judges only records older than that; a query inserted afterwards is never looked at. The ids are walked in batches of 5000 instead of one distinct() and one $nin over the whole collection. MemoryStorage looked for orphans in the wrong collection. STORAGE_MONGODB_COMPACT_AFTER_CLEANUP is described in docs/TUNING.md.
Contributor
Author
|
Live verification on the MongoDB-backed instance (server and worker restarted on this change): Planted the residue the issue is about: two query functions whose query sample does not exist, two query disassembly documents (one for an orphan function, one for no function at all), and a failed Scheduled Afterwards |
The branch had fallen 50 commits behind main, which in the meantime landed a large storage-layer rewrite (#44, danielplohmann#137, danielplohmann#142, danielplohmann#149-danielplohmann#152, danielplohmann#154, danielplohmann#157, danielplohmann#158, the explicit band projection and the picblockhash inverted index). All four conflicts turned out to be two independent additions landing at the same place, so each was resolved by keeping both sides; nothing was dropped from either side, and no logic was rewritten. Verified mechanically: the diff of this merge against origin/main is byte-for-byte the branch's own diff against the merge base, so main's 50 commits and the branch's two arrive intact. mcrit/storage/StorageInterface.py Both sides appended an abstract method after addFamily(): the branch's deleteOrphanedQueryData()/compactQueryCollections() (danielplohmann#68) and main's recomputeFamilyStats() (danielplohmann#151). They share only the trailing `raise NotImplementedError`, which is what made git call it a conflict. Kept all three declarations. mcrit/storage/MemoryStorage.py Same shape: the branch's two in-memory implementations against main's recomputeFamilyStats() plus the minhash-repair block from danielplohmann#142 (deleteMinHashesForSample, deleteAllMinHashes, setMinHashVersionForSamples, getSamplesWithStaleMinHashes). Disjoint concerns, both kept. The branch's implementation still holds against main's MemoryStorage: _query_functions and _query_samples survived the rewrite unchanged, and query entries carry their disassembly inside the FunctionEntry here, so "query_xcfg": 0 stays correct. mcrit/storage/MongoDbStorage.py The branch's deleteOrphanedQueryData()/compactQueryCollections() against main's race-tolerant recomputeFamilyStats() (danielplohmann#151), both inserted between modifyFamily() and deleteFamily(). Both kept. Checked against what main now relies on in the deletion paths, since that is where this could have gone wrong quietly: - main's picblockhash inverted index (danielplohmann#154) is maintained only for corpus functions - addSmdaReport's isQuery branch never calls _addToPicBlockHashIndex, and deleteSample returns before _removeSampleFromPicBlockHashIndex for sample_id < 0. Orphaned *query* functions therefore have no posting lists to withdraw, and the cleanup correctly leaves the index alone. - main's family counters (danielplohmann#151) are aggregated over `samples`/`functions` only; query samples are family_id 0 in `query_samples` and are not counted, so deleting orphaned query data must not - and does not - touch them. - the collections the cleanup walks (query_samples, query_functions, query_xcfg) and the _id-is-the-function-id shape of query_xcfg are exactly what main's danielplohmann#137 xcfg split still produces, so the orphan predicate is unchanged in meaning. tests/testStorage.py The branch's two mongo tests for the new storage methods against main's five family-statistics tests plus the _driftFamilyCounters/_numBandEntries helpers, all appended to MongoDbStorageTest. Every test from both sides was kept - no test was dropped to settle a conflict. mcrit/Worker.py auto-merged, and the result is right rather than merely conflict-free: main's danielplohmann#150 job reclamation releases a dead worker's job and, once it is out of attempts, leaves it in state "failed" with started_at set, finished_at unset and no result at all. doDbCleanup used to do SampleEntry.fromDict(result["info"]["sample"]) unconditionally and would now crash on precisely those jobs, so main made the bug this branch fixes strictly easier to hit; the branch's _querySampleOfJob() returning None is what the new reclamation path needs. Validation on the merge result: ruff format (142 files unchanged), ruff check and ty all clean; 190 unit tests pass (187 on main plus the branch's three in testWorkerCleanup.py) and 287 pass against MongoDB (282 on main plus those three and the branch's two in testStorage.py). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011EAW1DRkBwmjZtGzQ5pgDA
Two follow-ups on the orphan cleanup, both found while reviewing this branch against main rather than reported. deleteOrphanedQueryData took its boundary from the newest query function and returned early when there was none, so with query_functions empty the orphaned query_xcfg documents were never collected - permanently, since nothing else looks at them. That state is reachable rather than theoretical: addSmdaReport writes the disassembly (_insertXcfgDocuments) before the functions (_dbInsertMany), so an insert interrupted between the two leaves exactly this residue, and once the queries around it are deleted the collection the boundary came from is empty. Reproduced against mongo: two blobs, cleanup answers query_xcfg 0, and answers 0 again on every later run. With no function id to bound by, an existing query *sample* is what proves an insert may be in flight - the sample is written first of the three, so an empty query_samples means nothing has reached the step that writes disassembly. Only then is every query_xcfg document collected. The second fix is the two distinct() calls that produced the sample ids. The docstring promised "no single command carries the whole collection" and these were the exception: distinct answers with one document and fails past MongoDB's 16 MiB cap, at roughly 1.3M distinct ids. They are now one $group aggregation read through a cursor, batched at _ORPHAN_BATCH_SIZE, with the query_samples lookup done per batch instead of snapshotted whole. Reading the samples later is the safe direction: a sample written meanwhile is found (fewer false orphans), and one deleted meanwhile took its functions with it anyway. Three tests. The first two fail on the previous implementation - the blobs survive the cleanup and the in-flight guard has nothing to guard. The third walks more distinct sample ids than one batch holds with a live sample in the middle of them; it guards the new cursor path rather than reproducing a failure, since the 16 MiB ceiling needs more ids than a test can reasonably insert. 290 pass against mongo, up from 287. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011EAW1DRkBwmjZtGzQ5pgDA
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
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.
Closes #68 (considerations for DbCleanup: orphan query samples and functions without a job, a compact afterwards).
What the cleanup did, and the bug in it
doDbCleanupdeleted query samples and query jobs older thanSTORAGE_MONGODB_CLEANUP_TTL, which already covers query samples whose job is gone (they age out by their own timestamp). It readresult["info"]["sample"]off every finished and failed query job, though, and a failed job that died before matching has no result, so the first such job took the whole cleanup down with aTypeError. On an instance withSTORAGE_MONGODB_ENABLE_CLEANUPthat means the cleanup never completes again once one query job has failed early.Changes
finished_at, failed ones bystarted_at, a job that never started counts as old.deleteOrphanedQueryData()on the storage removes the query functions whose query sample is gone and the query disassembly (query_xcfg) whose function is gone: what an interrupted deletion or a job deleted without its sample leaves behind. The query collections are bounded by the TTL, so listing their ids is affordable; the memory backend drops orphaned query functions the same way.STORAGE_MONGODB_COMPACT_AFTER_CLEANUP(default off) runs MongoDB'scompactonquery_samples,query_functions,query_xcfg,fs.filesandfs.chunksafterwards, so the freed space goes back to the file system. It needs the compact privilege; a refusal is reported per collection, not raised, since the cleanup itself has succeeded by then.Verification
tests/testWorkerCleanup.py(3 tests, worker with fakes): an old failed job without a result is deleted and a recent one kept, a job that never started counts as old; a recent job protects its sample while an old job takes its old sample along and a recent sample is kept; orphan removal always runs and compaction only when configured, with both reflected in the report.tests/testStorage.py(Mongo): orphaned query functions and disassembly are deleted while the query sample's own are kept, and a second run finds nothing; compact answers per collection against the real MongoDB.ruff check,ruff format --check,ty checkclean.