Recurse delete fan-out to every directory, not just the top-level orphan - #68
Merged
Merged
Conversation
Production incident: a delete pass finished all its other work and then spent 6 hours on its last 2 shards, together accounting for ~14M files and directories. Root cause: DELETE fan-out only probed the path named directly in a shard's paths[] for delete_split_threshold — everything rm_tree found while recursing beneath that was removed depth-first with no further splitting, regardless of size. A tree where no single directory (at any depth) individually exceeded the threshold, but the whole subtree summed into the millions, never split at all. agent/src/delete.c: remove_object (renamed from remove_orphan) is now called for every directory entry rm_dir_contents finds during descent, not just the shard's own top-level paths — so a pathological subdirectory nested arbitrarily deep gets streamed out via stream_delete_split too, same as a top-level one. Reuses the probe's own directory handle (rewinddir, or dup+close on the pathological path) instead of a second openat, so this doesn't double the syscall cost at every level. This surfaced a real completion-ordering bug the flat case couldn't: a batch shard that hands a name off to its own nested delete_groups group reports itself done immediately (correctly — it has no more work), which could let its own parent's group close and rmdir before the nested child had actually finished being removed. Fixed with a new pending_children counter on delete_groups: registerPendingChildTx links a newly-created nested group to its parent (derived from the rel_path string, no explicit column needed); closeDeleteGroupTx decrements it on the child's own closure and walks back up the chain, re-checking closeability at each level. Added delete_fanout_nested_e2e.sh: a tree of 40 subdirectories (20 files each, none individually over threshold) that only fans out correctly if every directory is checked during descent, not just the one named in the shard's own paths[] — the existing flat-directory e2e test structurally cannot exercise a nested hand-off at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm
7 tasks
srhoods
added a commit
that referenced
this pull request
Aug 11, 2026
…g_children delete_groups' CREATE TABLE IF NOT EXISTS predates both columns (done_streaming from #67, pending_children from #68/this PR) and is a no-op against an already-existing table, so a coordinator whose data-dir was created by an older binary hit "SQL logic error: no such column: pending_children" on its very first delete pass after upgrading — reported live against exactly that scenario. Neither column had a corresponding ALTER TABLE entry in the migrations slice. Added both, plus a regression test that builds a pre-migration delete_groups table by hand and confirms Open (the real migration path) adds both columns and the table is fully usable afterward. Verified end-to-end against a fresh coordinator combining both fan-out mechanisms (a wide directory nested inside a 77-way branching tree, matching the reported production shape) with the exact tuning values reported: delete_split_threshold=1000, delete_split_batch=2500, delete_shard_budget=1000 — fan-out fires, the tree is fully removed, and content stays intact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm
srhoods
added a commit
that referenced
this pull request
Aug 12, 2026
…rectory (#69) * Add work-budget-based DELETE fan-out for trees with no single wide directory The prior fix (#68) caught a directory that is itself WIDE at any depth, but a real production tree hit a shape that mechanism structurally cannot see: 77 top-level branches, several levels deep, every individual directory comfortably under any threshold. No directory anywhere in the tree was ever wide enough to trip delete_split_threshold, so the whole multi-million-object tree ran serially inside 2 shards. Adds tuning.delete_shard_budget (default 250000, objects removed), mirroring the scan walker's shard_budget/queue_split: agent/src/ delete.c decrements a per-shard budget on every object removed, threaded through the recursive descent. Once exhausted, every not-yet-opened subdirectory is handed off as its own new top-level DELETE shard (ShardSplit.delete_subdirs) instead of being recursed into. This introduced two more completion-ordering bugs, both caught locally by the new delete_fanout_budget_e2e.sh before reaching CI: 1. A handed-off shard's own single-shard "group" was seeding a redundant cleanup rmdir on top of its own ordinary removal — fixed with a NoSelfCleanup flag on DeleteGroupTotal. 2. A directory that was never itself handed off, but merely an ancestor of one deep in an otherwise-inline rm_tree recursion, had no way to know a descendant was still mid-removal elsewhere, and rmdir'd itself prematurely. Fixed by having the agent track this locally (no coordinator round-trip): a handoff anywhere inside an in-progress rm_tree call propagates back up the C call stack, and every ancestor that sees a deferred descendant skips its own rmdir too, reporting the deferred paths once in its own ShardResult (deferred_rmdirs, new proto field). Coordinator-side, registerPendingChildTx now walks every ancestor up to the top-level orphan path (not just the direct parent) the first time anything beneath it is handed off, stopping the climb the moment it reaches an already-tracked level — an actual over-counting bug (pending_ children reaching 69 instead of 10) was caught and fixed during this same investigation before that stop condition was added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm * Add missing schema migrations for delete_groups.done_streaming/pending_children delete_groups' CREATE TABLE IF NOT EXISTS predates both columns (done_streaming from #67, pending_children from #68/this PR) and is a no-op against an already-existing table, so a coordinator whose data-dir was created by an older binary hit "SQL logic error: no such column: pending_children" on its very first delete pass after upgrading — reported live against exactly that scenario. Neither column had a corresponding ALTER TABLE entry in the migrations slice. Added both, plus a regression test that builds a pre-migration delete_groups table by hand and confirms Open (the real migration path) adds both columns and the table is fully usable afterward. Verified end-to-end against a fresh coordinator combining both fan-out mechanisms (a wide directory nested inside a 77-way branching tree, matching the reported production shape) with the exact tuning values reported: delete_split_threshold=1000, delete_split_batch=2500, delete_shard_budget=1000 — fan-out fires, the tree is fully removed, and content stays intact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.
Summary
paths[]— a nested subdirectory found duringrm_tree's recursion was never checked againstdelete_split_threshold, no matter how large.remove_object(renamed fromremove_orphan) is now applied at every directory a removal touches during descent, not just the shard's own top-level orphan path.delete_groupsgroup reported itself done immediately, which could let its parent's group close andrmdirbefore the nested child actually finished. Fixed with apending_childrencounter andcloseDeleteGroupTx's upward chain walk.delete_fanout_nested_e2e.sh, which builds a tree of many individually-small subdirectories (none over threshold alone) — the only way to exercise the nested hand-off path at all.Test plan
go test -count=1 ./...— all greenmake -C agent test— all greenTestDeleteGroupNestedChildBlocksParentClose— verified it fails against the pre-fix code (parent's group closed prematurely), passes with the fixdelete_fanout_e2e.sh(flat case) — still passesdelete_fanout_nested_e2e.sh(new nested case) — passes: all 40 subdirectories, all 800 files, and the top orphan directory itself fully removed🤖 Generated with Claude Code
https://claude.ai/code/session_01PsdNZLfmAFrMX2VUtkLtmm