Repository navigation
feat(backend): orphan-storage detector framework + code-source clone detector (#17038, #17039) - #17050
Closed
mrveiss wants to merge 9 commits into
Closed
feat(backend): orphan-storage detector framework + code-source clone detector (#17038, #17039)#17050mrveiss wants to merge 9 commits into
mrveiss wants to merge 9 commits into
Conversation
…detector (#17038, #17039) A backend framework in which a data-owning module registers a read-only orphan-storage detector (services/orphan_storage.py). Detection lives with the owner of the data, as #17038's design requires: this module only aggregates and dispatches, holding no storage knowledge of its own. api/codebase_analytics/orphan_clone_detector.py is the first detector: a code-source clone directory with no matching source record -- exactly the #17036 orphan (f5673506..., a 290 MB clone from a failed delete). Excludes anything younger than AUTOBOT_ORPHAN_GRACE_HOURS (default 24h, clamped to 1-720), and re-checks orphan status and the grace period again at delete time, so a directory whose record reappears -- or that's no longer past the grace window -- is refused rather than trusting a stale listing. GET /api/admin/orphan-storage (admin-only) previews candidates across every registered detector. No bare admin DELETE route is exposed: the owner's rule requires every removal to go through a human-approved review queue (#17043), so the delete-through-owner function exists and is fully tested, but nothing calls it yet outside tests until that queue's execution hook lands. Also fixes #17036: source_service.py's and endpoints/sources.py's clone-directory removal no longer swallows shutil.rmtree with ignore_errors=True. A failed removal is logged with the path and error and reported (the delete returns False rather than silently succeeding); the source record is marked CLEANUP_FAILED rather than dropped while the directory survives. The re-clone path in _do_sync skips the clone attempt entirely when it can't clear a stale directory first, instead of writing into whatever's left of it.
Contributor
|
Warning Review limit reachedNext included review available in 55 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sync_orphan_clone_detector's list_sources()/get_source() both return an empty/None result on a Redis outage -- indistinguishable, from the return value alone, from "there genuinely are no sources" or "this one genuinely has no record". Both the listing and delete-time re-check now call the registry directly first and refuse (return no candidates / refuse the delete) when it's unreachable, rather than reading the outage as "zero sources" and offering every clone on disk as orphaned. A human could otherwise be asked to approve deleting all of them during a Redis blip.
…17039 review) Second independent-review round found the c3777bf fix incomplete: _registry_reachable() checked once, then list_sources()/get_source() each acquired their own client and could still return an empty/None result mid-loop on a partial failure (e.g. the breaker opening between the check and the call, or one id's own fetch failing) -- an id could silently drop out of known_ids and its clone would be offered as an orphan even though its record still exists. - source_storage.py: new registered_source_ids() reads the raw index set (smembers) directly and RAISES RegistryUnavailable when the client is None, instead of returning an empty collection. Membership only, no per-id deserialisation, so one record's own fetch failing can never drop its id. - orphan_clone_detector.py: _list_candidates() calls it and lets the exception propagate (never swallowed locally); _delete()'s re-check catches it explicitly and refuses rather than treating "unreachable" as "confirmed not present". - services/orphan_storage.py: list_all_candidates() now returns an OrphanListing (candidates + a ProviderStatus per detector), so a detector that raised is reported as unavailable with why, never read as "found nothing" (MEASUREMENT_DISCIPLINE: a report must tell nothing-found apart from did-not-look). delete_candidate() also catches an unexpected exception from the detector's own delete now, so the safe outcome is always "nothing was deleted", never an unhandled exception. - admin_orphan_storage.py / schemas_orphan_storage.py: GET /api/admin/orphan-storage's response gained provider_statuses, tested end to end -- an outage names the unavailable provider instead of returning an empty, success-shaped list. - endpoints/sources.py: the DELETE response includes status/ error_message inline on a cleanup failure instead of a bare 200/false. - New test: endpoints/sources_reclone_test.py, closing the coverage gap the same review found on _do_sync's re-clone path (the other #17036 site) -- untested until now.
2 of 5 tasks
Closed
3 tasks done
3 tasks
This was referenced Sep 18, 2026
Merged
…ests (#17065) Round-3 review of #17050 approved the design and asked for two small in-scope fixes before merge: - Cleanup and registry-outage errors returned raw exception text, which can carry the clone directory's absolute host path (OSError) or a Redis host/port (any other exception). New safe_error_reason() (one copy in services/orphan_storage.py for the framework, one in source_service.py to avoid a layering-backwards import) returns OSError.strerror when the OS itself raised it (shutil.rmtree always does), str(exc) for a hand-raised message-only OSError (which never carries a filename to begin with), and the exception's class name for anything else, since a non-OSError's own message text can't be trusted the same way. The full exception stays in the server log, never in a value an API response returns. Closes #17065. - registered_source_ids() itself was only ever exercised through monkeypatched detector tests -- new source_storage_test.py drives it directly: get_async_redis_client returning None raises RegistryUnavailable, and a fake client's byte-string smembers() result decodes correctly.
This was referenced Sep 18, 2026
…ponse (#17065) Two local copies of safe_error_reason had drifted to different signatures (OSError vs BaseException) in source_service.py and services/orphan_storage.py. Moved the canonical version into the existing autobot_shared/security/safe_response.py (#1721) module and updated all 4 call sites to import from there instead of forking it again.
mrveiss
added
the
land-next
This was referenced Sep 18, 2026
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
Auto-regenerated (py3.14) to match the backend and/or SLM backend schema so the required verify-generated-types gate(s) pass. Triggered by auto-fix-generated-types.yml.
This was referenced Sep 18, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # failure (#17036) -- surface it instead of a bare 200/false. | ||
| body["status"] = source.status.value | ||
| body["error_message"] = source.error_message | ||
| return JSONResponse(body) |
This was referenced Sep 18, 2026
3 tasks done
This was referenced Sep 19, 2026
mrveiss
added a commit
that referenced
this pull request
Sep 19, 2026
Owner
Author
|
Carried into vehicle #17119 (member head |
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.
Thinking Path
Detection lives with the owner of the data, per #17038's design: a data-owning
module builds its own read-only detector and registers it into a small
framework that only aggregates and dispatches. The first detector (code-source
clones) exists to catch exactly the #17036 orphan (
f5673506..., a 290 MBclone from a failed delete on 2026-07-24), which stays on disk until the
Data-hygiene page (#17040) exists -- nobody runs
rmon the host.The owner's binding rule (every removal previewed and approved by a human,
never automatic) landed after this issue's AC was written, and was then
extended further (an always-available review queue, #17043) mid-implementation.
Per 66's explicit ruling: detection and deletion don't depend on #17043, so
this PR builds both fully, but does not expose a bare admin-gated DELETE
route -- that would let an admin skip the review queue #17043 is building.
The delete-through-owner function exists, is fully tested, and is what
#17043's approved-cleanup execution will call once it lands.
What Changed
services/orphan_storage.py(new): the detector framework --OrphanCandidate,DeleteResult,OrphanDetector,register_detector,list_all_candidates()(one detector's failure never hides the rest),delete_candidate(),orphan_grace_period_hours()(
AUTOBOT_ORPHAN_GRACE_HOURS, clamped to 1-720h, default 24h).api/codebase_analytics/orphan_clone_detector.py(new): the code-sourceclone detector. Lists directories under
CODE_SOURCES_BASEwith nomatching
CodeSourcerecord, past the grace period. Delete re-checksorphan status and the grace period again at execution time -- a directory
whose record reappeared, or that's no longer past the window, is refused.
locationin every candidate is logical ("code-sources/<id>"), never ahost path.
api/admin_orphan_storage.py+api/schemas_orphan_storage.py(new):GET /api/admin/orphan-storage, admin/superadmin only (require_role),read-only preview across every registered detector, with totals.
api/codebase_analytics/source_service.py::delete_source_and_cleanup:a failed
rmtreeis caught, logged with the path and error, and therecord is marked
SourceStatus.CLEANUP_FAILED(new enum value) with theerror instead of being dropped while the directory survives. Returns
Falseso the caller sees the partial failure.api/codebase_analytics/endpoints/sources.py::_do_sync's re-clone path:a failed clear of a stale directory is logged and reported as a sync
error, and the clone attempt is skipped entirely rather than writing
into whatever's left of a partially-cleared directory.
initialization/router_registry/core_routers.py,next to the existing
admin_orphan_repairentry (fix(security): break-glass repair of a genuinely unreachable knowledge fact or envelope secret, through each type's own service (#15779, #16940) #16927's sibling feature).AUTOBOT_ORPHAN_GRACE_HOURSregistered inautobot_shared/env_registry_backend_services.py;ENV_VARS.mdregenerated.Verification
test_refuses_when_a_record_now_references_the_directory,test_refuses_when_no_longer_past_the_grace_period.test_a_directory_younger_than_the_grace_period_is_excluded,plus a clamp test for a 0/negative env value.
test_a_failed_removal_is_reported_never_swallowed(detector),test_a_failed_rmtree_keeps_the_record_marked_cleanup_failed(source_service),matching bug(codebase-analytics): a deleted code source can leave an orphan clone on disk — removal uses rmtree(ignore_errors=True), and nothing sweeps orphans #17036's own AC for both the source-delete and detector-delete paths.
test_a_non_admin_is_refused(403/401), verifiedthrough the real
require_role/auth_rbacchain, not a stubbed dependency.test_a_directory_with_no_source_record_past_the_grace_period_is_listed.python3 scripts/check_python_file_size.py --audit-ceilings-- clean.python3 pipeline-scripts/check_env_var_registry.py-- exit 0.bandit -c .banditclean on every changed file.jscpd@5.0.6reproduction of the duplication-guard's exact invocation:11718 duplicated lines, unchanged from
main's own baseline.detect-secrets scan(no baseline write): no findings.bash pipeline-scripts/detect-hardcoded-values.sh: clean.Risks
detector or any future detector -- intentional, per the review-queue rule;
refactor(approvals): consolidate the general and LLC approval systems into one platform approval service with optional company scope (#17038) #17043 wires the execution path.
to this PR's
services.orphan_storage.delete_candidate) removes it throughan approved request -- not removed by this PR itself.
Model Used
Claude Sonnet 5 (claude-sonnet-5)
Issue Link
Refs #17038, Refs #17039, Closes #17065
Single-issue rationale
This PR delivers #17039's detection and #17036's fixes in full, but does not
close #17039 -- its own AC (an admin-gated delete endpoint) is superseded by
the owner's later review-queue rule, and the delete-through-owner path it
asks for won't be reachable until #17043's approval execution calls it.
Closing #17039 happens once that hand-off lands.
Changelog fragment
changelog/unreleased/17039-orphan-storage-detector.mdChecklist
ENV_VARS.md)🤖 Generated with Claude Code