Skip to content

fix: reduce memory usage from unbounded state and eager imports#64

Open
27Bslash6 wants to merge 1 commit into
blazickjp:mainfrom
27Bslash6:upstream/fix-memory-leaks
Open

fix: reduce memory usage from unbounded state and eager imports#64
27Bslash6 wants to merge 1 commit into
blazickjp:mainfrom
27Bslash6:upstream/fix-memory-leaks

Conversation

@27Bslash6

Copy link
Copy Markdown

Summary

Running this server in k8s, it was OOMing every ~3 hours at a 2Gi memory limit — with zero papers downloaded. Root causes:

  • pymupdf4llm + fitz (~150-200MB of C extensions) imported eagerly at module level even when the server only handles search queries
  • arxiv.Client() created fresh on every single request (new HTTP session/connection pool each time, never closed)
  • conversion_statuses dict grows without bound — entries added on every download, never removed
  • PDF files never cleaned up after conversion (line 80 had a comment saying "Clean up PDF" but did nothing)
  • ResearchContext.explored_papers / paper_analyses dicts grow without bound
  • PaperManager class in resources/papers.py is never imported by server.py but pulls in pymupdf4llm and arxiv at module level
  • black>=25.1.0 is in runtime dependencies instead of dev optional-dependencies

Changes

  1. Lazy-import pymupdf4llm and fitz — moved inside convert_pdf_to_markdown(), the only function that uses them. Server startup no longer loads ~200MB of C extensions.
  2. Shared arxiv.Client() singletonget_arxiv_client() in config.py, reused by download/search/list_papers instead of creating one per request.
  3. Bounded conversion_statuses — entries removed in finally block after conversion completes (success or error).
  4. PDF cleanuppdf_path.unlink(missing_ok=True) after successful conversion + gc.collect() to nudge C-level allocator.
  5. Capped ResearchContext — both dicts capped at 50 entries (FIFO eviction).
  6. Removed dead PaperManagerresources/papers.py gutted (the class was never wired into server.py).
  7. Moved black to dev deps — was pulling in black + its dependency tree at runtime for no reason.

Test plan

  • All 32 existing tests pass
  • Verified fitz and pymupdf4llm are NOT in sys.modules after importing server.py
  • black --check passes on all files

- Lazy-import pymupdf4llm and fitz (150-200MB C extensions) only when
  conversion is needed, not at server startup
- Share a single arxiv.Client() via get_arxiv_client() instead of
  creating a new one per request
- Bound conversion_statuses dict by removing entries after completion
- Actually clean up PDF files after conversion (was a TODO)
- Cap ResearchContext dicts at 50 entries to prevent unbounded growth
- Remove dead PaperManager class (never imported by server.py) that
  pulled in pymupdf4llm and arxiv at module level
- Move black from runtime deps to dev (was installing black + deps
  in production for no reason)

All 32 tests pass.
blazickjp added a commit that referenced this pull request Apr 3, 2026

@JiwaniZakir JiwaniZakir left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The eviction logic added in prompts/handlers.py is duplicated verbatim at two callsites (explored_papers and paper_analyses), which is worth extracting into a small helper like _evict_oldest(d, max_size) to avoid divergence if _MAX_CONTEXT_ENTRIES semantics ever need to change. Additionally, the cap strategy silently drops the oldest entry with no logging, making it hard to diagnose memory-pressure behavior in production — a single logger.debug line would go a long way.

The get_arxiv_client() singleton in config.py isn't safe under concurrent initialization: two coroutines could both observe _arxiv_client is None before either finishes constructing it, particularly if arxiv.Client() ever suspends or if the server runs tasks in a thread pool. Using a module-level threading.Lock or an asyncio.Lock (initialized at import time, not inside the function) would close this race.

The gutted resources/papers.py is now just a module-level docstring explaining that the class was removed. Keeping an empty file "for potential future use" is unnecessary noise — git history preserves the implementation if it's ever needed again, and the now-empty __init__.py export removal already signals the intent clearly.

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.

2 participants