Skip to content

Conversation

@Ojaybee
Copy link

@Ojaybee Ojaybee commented Jun 12, 2025

Refactors media_sync_daemon.py to eliminate unbounded memory growth by rewriting the main loop.

Tested on Lightsail t3.nano with persistent background sync.

Note: Block I/O still increasing unchecked.

image

@PeterPetrik PeterPetrik requested a review from JanCaha June 12, 2025 12:32
@JanCaha
Copy link
Collaborator

JanCaha commented Jun 20, 2025

@varmar05 To me this looks ok.

I should not be really necessary to manually call the gc but I can image that it might help and prevent some issues here.

The logging seems to impove with this as well.

@JanCaha JanCaha requested a review from varmar05 June 20, 2025 09:54
Copy link
Contributor

@varmar05 varmar05 left a comment

Choose a reason for hiding this comment

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

Thanks @Ojaybee for your contribution. I like the logging improvement and bit of refactoring. However I would like to understand more the memory part.

logger.info("Sync complete.")

# Force garbage collection
gc.collect()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know what was the issue with memory (maybe we could fix the source of the issue)? As gc should be automatic we should probably know why to force it.

sys.exit(1)

print("Logging in to Mergin...")
logger.info("Logging in to Mergin...")
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to update Mergin... to Mergin Maps server ... in all occurrences for better clarity

if delta.total_seconds() < 3600:
logger.info("Refreshing Mergin auth token...")
mc = create_mergin_client()
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to me too broad, can you narrow it down to MerginClient error or something similar?

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.

4 participants