Conversation
… result doDbCleanup deleted query samples and query jobs older than the TTL, and nothing else (familiary#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.
Collaborator
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, familiary#137, familiary#142, familiary#149-familiary#152, familiary#154, familiary#157, familiary#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() (familiary#68) and main's recomputeFamilyStats() (familiary#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 familiary#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() (familiary#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 (familiary#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 (familiary#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 familiary#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 familiary#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
# Conflicts: # docs/TUNING.md
…S to the queue A query is inserted as its sample, then the disassembly of every function, then the functions. The orphan walk judged the disassembly against the newest query function, so two queries inserted at once that finished out of order had the disassembly of the one still writing its functions deleted under it: the second one's functions moved the boundary past the first one's ids. The case without any query function checked for a query sample and then deleted every disassembly document, with room for an insert in between. A query sample now records how many functions its insert writes (num_query_functions). The disassembly is judged behind the newest disassembly document, taken before anything else, and only while no query sample is short of its functions; that covers the empty case too. A sample that stays short is an insert that died, and its disassembly goes in the run that deletes the sample. Query samples written before this have no count and hold the judgment back until they expire. compactQueryCollections no longer compacts fs.files and fs.chunks. They are the job queue's GridFS, which lives in the queue's database: the storage database reached it only when both keep their default name. On this branch, deleting a job also left its chunks behind, so compacting them returned nothing. Reclaiming GridFS is left to the queue. docs/TUNING.md says so, and no longer claims compact blocks writes, which it has not done since MongoDB 4.4. The memory queue raised on deleting a job that left no result, which the query cleanup now does for old failed jobs; it deletes such a job now. The changelog gains entries for this branch under Unreleased.
…ix does The same two lines familiary#80 changes in LocalQueue.delete_job, so that merging either branch after the other does not conflict over a comment.
Dropping fs.* from the compaction assumed the queue's GridFS never lives in the storage database, but both database names default to mcrit, so in a default deployment it does, and the results of the query jobs the cleanup deletes are the bulk of what it frees. fs.files and fs.chunks are compacted again when the queue keeps its data on the same server, port and database, and left alone when it has a database of its own, which this handle cannot reach.
Collaborator
Author
|
Merged |
This branch has not been deployed
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
1 participant
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.