Skip to content

Clean up the query data nothing refers to, and survive jobs without a result (#68) - #176

Open
r0ny123 wants to merge 4 commits into
danielplohmann:mainfrom
r0ny123:fix/68-cleanup-orphans
Open

r0ny123 wants to merge 4 commits into
danielplohmann:mainfrom
r0ny123:fix/68-cleanup-orphans

Conversation

@r0ny123

@r0ny123 r0ny123 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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

doDbCleanup deleted query samples and query jobs older than STORAGE_MONGODB_CLEANUP_TTL, which already covers query samples whose job is gone (they age out by their own timestamp). It read result["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 a TypeError. On an instance with STORAGE_MONGODB_ENABLE_CLEANUP that means the cleanup never completes again once one query job has failed early.

Changes

  • A job without a result is nothing to protect and nothing to collect: it is deleted once older than the TTL and kept otherwise. Finished jobs are dated by finished_at, failed ones by started_at, a job that never started counts as old.
  • After the TTL pass, 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's compact on query_samples, query_functions, query_xcfg, fs.files and fs.chunks afterwards, 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.
  • The job now answers a report: samples and jobs deleted, orphans removed, and the compact outcome when it ran.

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.
  • Full suite: 208 passed; ruff check, ruff format --check, ty check clean.
  • Live verification on the MongoDB-backed instance follows in a comment.

… 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.
@r0ny123

r0ny123 commented Sep 6, 2026

Copy link
Copy Markdown
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 getMatchesForUnmappedBinary job from nine days ago with attempts_left: 0 and no result, which is the shape that used to take the cleanup down.

Scheduled doDbCleanup through the index. The job finished and answered:

{'num_query_samples_deleted': 0, 'num_query_jobs_deleted': 1, 'orphans': {'query_functions': 2, 'query_xcfg': 2}}

Afterwards query_functions and query_xcfg were empty, the failed job was gone, and the four corpus samples with their 1158 functions were untouched.

r0ny123 and others added 2 commits September 12, 2026 20:58
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
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.

Considerations for DbCleanup

1 participant