Skip to content

Fix RecursionError for duplicate files#133

Open
dvdl16 wants to merge 3 commits into
agritheory:version-15from
Starktail:version-15
Open

Fix RecursionError for duplicate files#133
dvdl16 wants to merge 3 commits into
agritheory:version-15from
Starktail:version-15

Conversation

@dvdl16

@dvdl16 dvdl16 commented Jun 20, 2026

Copy link
Copy Markdown

Fixes #132

  1. Guard associate_files() behind a flag so a save triggered from within association does not re-run association:
def validate(self):
    # guard against recursion: associate_files() can save another File, re-entering validate
    if not self.flags.associating_files:
        self.associate_files()
    ...
  1. Set the flag before saving the duplicate in associate_files():
existing_file.flags.associating_files = True
existing_file.save()
  1. Avoid duplicate file_association child rows - only append the association if the same link_doctype / link_name isn't already present:
already_linked = any(
   assoc.link_doctype == attached_to_doctype and assoc.link_name == attached_to_name
   for assoc in existing_file.file_association
)
if not already_linked:
   existing_file.append(
       "file_association",
       add_child_file_association(attached_to_doctype, attached_to_name),
   )

This breaks the recursion while still performing the intended one-pass association, and prevents redundant association rows from accumulating.

dvdl16 added 3 commits June 20, 2026 10:22
`associate_files()` is called from `validate`, and for Files sharing a `content_hash` it calls `.save()` on the existing duplicate row. That save re-enters `validate` -> `associate_files`, which saves back into the original file, producing an endless A->B->A loop (maximum recursion depth exceeded).

- Guard `associate_files()` in `validate` with a new `associating_files` flag so a save triggered from within association does not re-run association. - Set `existing_file.flags.associating_files = True` before saving the duplicate so the recursion is broken. - Skip appending a `file_association` row that already links the same doctype/name, avoiding duplicate child rows on repeated associations.
@dvdl16

dvdl16 commented Jun 20, 2026

Copy link
Copy Markdown
Author

@agritheory Thank you for this app! As I'm not a collaborator, I don't think the failing actions have access to the necessary env vars. Let me know how you'd like me to proceed

@agritheory agritheory requested a review from lauty95 June 24, 2026 13:07

@lauty95 lauty95 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dvdl16 thanks for this fix!

The tests pass locally, but I noticed that this PR doesn't add regression tests for this scenario.

Could you add a couple of tests for this?
Two tests that would help:

  • Create two File docs with identical content_hash (via db_insert(), bypassing the merge-on-insert), then .save() one of them and assert it completes without RecursionError.
  • Call associate_files() twice with the same link_doctype/link_name and assert file_association doesn't grow a duplicate row (covers the new already_linked guard).

To run the suite locally:

Activate the bench virtualenv: source env/bin/activate
Make sure sites/currentsite.txt contains your site name
Run: pytest --disable-warnings -s from apps/cloud_storage

@dvdl16

dvdl16 commented Jul 7, 2026

Copy link
Copy Markdown
Author

Thank you @lauty95! Will do and revert back

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.

RecursionError: maximum recursion depth exceeded when saving a File that shares content_hash with another File

2 participants