Harden logging, API client, and storage integrations

[nevs55]

Summary

• harden logging setup and avoid circular config/logger imports
• centralize MemOS API client request/JSON handling while preserving test compatibility
• gate local Redis fallback, quote PostgreSQL schema identifiers, cap Qdrant pagination, and replace unsafe shell cleanup

Verification

• python -m py_compile src/memos/api/client.py src/memos/configs/base.py src/memos/log.py src/memos/mem_scheduler/webservice_modules/redis_service.py src/memos/vec_dbs/qdrant.py src/memos/graph_dbs/postgres.py evaluation/scripts/utils/mirix_utils.py
• pytest -q tests/api/test_client.py
• pytest -q tests/vec_dbs/test_qdrant.py tests/graph_dbs/test_search_return_fields.py
• pytest -q tests/api/test_cube_endpoints.py tests/api/test_server_router.py tests/api/test_mcp_serve.py tests/api/test_scheduler_handler_allstatus.py
• pytest -q tests/mem_scheduler/test_dispatcher.py tests/mem_scheduler/test_retriever.py
• pytest -q tests/mem_scheduler/test_scheduler.py
• docker build -f Dockerfile --load -t memos-memos:cfb92564 -t memos-memos:latest .

Docker image

• Local image built from commit cfb9256: memos-memos:cfb92564 / memos-memos:latest
• Image ID: sha256:dd21c260a80674f413aef314ef52cd520d4ec8fd4ad87d7a734eaa37a6bf93a2
• Platform: linux/amd64

[Memtensor-AI]

🤖 Open Code Review

Target: PR #2232
Task: 4df9e12d917d9d25
Base: main
Head: main

🔍 OpenCodeReview found 14 issue(s) in this PR.

---

1. src/memos/cli.py (L59-L60)

The ValueError raised for a detected zip-slip path is caught by the broad except Exception block on line 76, which prints a generic '❌ Error extracting examples' message and returns False. This means a zip slip attempt is indistinguishable from a network or I/O error. Consider re-raising or handling this specific security event separately — e.g. by moving the path-traversal check outside the try/except Exception scope, or catching ValueError explicitly before the broad handler.

💡 Suggested Change

Before:

                    if not extract_path.is_relative_to(dest_root):
                        raise ValueError(f"Unsafe zip path: {file}")

After:

                    if not extract_path.is_relative_to(dest_root):
                        print(f"❌ Unsafe zip path detected, aborting: {file}")
                        return False

---

2. src/memos/configs/base.py (L3-L6)

Minor import ordering issue: PEP 8 recommends keeping stdlib imports together before third-party imports. from typing import Any (stdlib) and import logging (stdlib) should both appear before import yaml (third-party), and within the stdlib group, all stdlib imports should be contiguous. Consider grouping them as:

import logging
import os
from typing import Any

import yaml

---

3. src/memos/api/client.py (L447-L466)

Double-retry bug: _request_json is called with retry_count=1 (single attempt), and on failure it raises a RequestException. The outer for retry in range(MAX_RETRY_COUNT) loop catches that exception and retries — so the file upload is retried, but all other exceptions (TypeError, ValueError from JSON parse, ValueError from build_file_form_params) are not caught by the outer loop and will propagate immediately without retrying OR logging the retry count correctly.

More importantly, the finally block closes the file handles after every attempt (correct), but build_file_form_params() is called fresh on each retry (reopening files), which is the right pattern. However, the exception handling gap means a TypeError from isinstance(response_data, requests.Response) check on line 462 — which can never be True when expect_json=True is passed since _request_json either returns a dict or raises — silently leaks. Consider removing the dead isinstance guard and consolidating retry logic entirely inside _request_json instead of the outer loop.

💡 Suggested Change

Before:

        for retry in range(MAX_RETRY_COUNT):
            file_params = []
            try:
                file_params = build_file_form_params()
                response_data = self._request_json(
                    method="post",
                    endpoint="add/knowledgebase-file",
                    operation="add knowledgebase-file form",
                    expect_json=True,
                    params=payload,
                    headers=headers,
                    files=file_params,
                    retry_count=1,
                    timeout=30,
                )
                if isinstance(response_data, requests.Response):
                    raise TypeError("Expected JSON response for knowledgebase-file form")

                return MemOSAddKnowledgebaseFileResponse(**response_data)
            except requests_exceptions.RequestException as e:

After:

        for retry in range(MAX_RETRY_COUNT):
            file_params = []
            try:
                file_params = build_file_form_params()
                response_data = self._request_json(
                    method="post",
                    endpoint="add/knowledgebase-file",
                    operation="add knowledgebase-file form",
                    expect_json=True,
                    params=payload,
                    headers=headers,
                    files=file_params,
                    retry_count=1,
                    timeout=30,
                )
                return MemOSAddKnowledgebaseFileResponse(**response_data)
            except (requests_exceptions.RequestException, ValueError) as e:

---

4. src/memos/api/client.py (L163)

Dead code: every exit path from the for loop either returns or raises before this line is reached. When retry_count >= 1 (enforced by the guard above), the last iteration always either returns a value or re-raises the caught exception (raise without argument in both the except ValueError and except RequestException branches on the last retry). This raise RuntimeError can never execute. While harmless today, it misleads readers into thinking there is a third exit path and could mask logic errors if the loop structure changes.

---

5. src/memos/api/client.py (L348-L352)

The url variable on line 348 is assigned but never used — _get_json_dict builds the URL internally via _build_url. This is dead code left over from the refactor. Also, the intermediate response_data variable adds no value; the method can be simplified to a direct return.

💡 Suggested Change

Before:

        url = f"{self.base_url}/get/memory/{quote(memid, safe='')}"
        response_data = self._get_json_dict(
            f"get/memory/{quote(memid, safe='')}", operation="get memory by ID"
        )
        return response_data

After:

        return self._get_json_dict(
            f"get/memory/{quote(memid, safe='')}", operation="get memory by ID"
        )

---

6. src/memos/api/client.py (L111)

Filtering out all None-valued kwargs is too aggressive. stream=False (a falsy but meaningful value) survives since it is not None, but any caller that explicitly passes stream=None — meaning "let the library default" — would have it silently dropped. More critically, params=None and json=None (intentional null body) are silently swallowed with no warning. This can cause subtle bugs if the method's signature is extended or called with explicit None arguments. Consider limiting the filter to a known set of optional keys rather than applying it globally, or removing it entirely.

---

7. src/memos/api/client.py (L121-L124)

Converting json= to data=json.dumps(...) for "legacy test compatibility" sets Content-Type to application/x-www-form-urlencoded (the requests default for data=) instead of application/json. This means the Content-Type: application/json header from self.headers is provided but the body is encoded as a raw string — whether the server gets the right content type depends on header merge order. More importantly, this is a leaky abstraction: production requests should use requests' native json= parameter (which sets both body and Content-Type correctly). If tests need to assert on data, the tests should be updated rather than silently changing production request encoding.

---

8. src/memos/log.py (L32-L33)

The except clause is broader than needed. Catching Exception here silently swallows any unexpected runtime error that occurs during import (e.g., a crash in the module's top-level initialization code), making it very hard to diagnose. Only ImportError (and its subclass ModuleNotFoundError) can legitimately be raised when an optional dependency is absent.

💡 Suggested Change

Before:

except Exception:
    _TimedRotatingFileHandler = TimedRotatingFileHandler

After:

except ImportError:
    _TimedRotatingFileHandler = TimedRotatingFileHandler

---

9. src/memos/log.py (L332-L335)

logging.exception() already implicitly captures the current exception and its traceback (equivalent to logging.error(..., exc_info=True)). Passing exc_info=exc explicitly is redundant here but, more importantly, this code is inside a plain except Exception as exc block — there is no active exception context at this point because no raise is used; Python's implicit sys.exc_info() may still hold the exception, but relying on both mechanisms is confusing. The clean approach is to either use logging.exception(...) alone (no exc_info keyword) and let it pick up the active exception, or switch to logging.error(..., exc_info=exc) if you want to be explicit.

💡 Suggested Change

Before:

                logging.getLogger(__name__).exception(
                    "Logging configuration via dictConfig failed; falling back to basicConfig",
                    exc_info=exc,
                )

After:

                logging.getLogger(__name__).exception(
                    "Logging configuration via dictConfig failed; falling back to basicConfig",
                )

---

10. src/memos/mem_scheduler/webservice_modules/redis_service.py (L346-L350)

The synchronous xread call with block=block_time (default 2000 ms) runs inside an async def function, which freezes the entire asyncio event loop for up to 2 seconds on every iteration. Any other coroutines sharing this event loop will be completely stalled during that window.

Similarly, _require_redis_connection() → self.redis → auto_initialize_redis() performs synchronous network ping() calls (and potentially time.sleep(0.5) when the local Redis fallback is active), all of which are blocking operations inside this async context.

Suggestion: Use an async Redis client (e.g. redis.asyncio.Redis) and await redis_conn.xread(...), or offload blocking calls via asyncio.to_thread() / loop.run_in_executor().

---

11. src/memos/mem_scheduler/webservice_modules/redis_service.py (L101-L104)

initialize_redis now returns None (implicitly) when ping() returns a falsy value (line 104), but returns self._redis_conn (a Redis object, not a bool) on success (line 110), and returns None again on ConnectionError (line 114). This method has no return-type annotation, but callers that rely on a truthy/falsy check will work by coincidence — a live Redis connection object is truthy. However, this mixed-type return (None vs Redis object) makes the contract ambiguous and fragile. Consider returning a consistent type: bool for status, or raising on failure instead of returning None.

---

12. src/memos/mem_scheduler/webservice_modules/redis_service.py (L362-L364)

When a RuntimeError is caught here (meaning _require_redis_connection() found self._redis_conn is None), the code correctly sleeps 5 seconds before retrying. However, on the next loop iteration _require_redis_connection() calls self.redis, which calls auto_initialize_redis() — a method that may itself block on network ping() calls for each configured strategy (config, env vars, local fallback) before returning False. With no backoff or cooldown guard on auto_initialize_redis, rapid successive failures will trigger full re-initialization attempts at the rate of only one 5-second sleep per failed RuntimeError, regardless of how expensive each attempt is. Consider tracking a _last_reconnect_attempt timestamp and skipping re-initialization if insufficient time has elapsed.

---

13. src/memos/mem_os/core.py (L233-L243)

The helper's None-resolution branch is dead code for get, update, and delete — those methods declare mem_cube_id: str (non-optional) in their signatures, so None can never arrive here from those callers. Only dump, load, and delete_all declare mem_cube_id: str | None = None.

The asymmetry also means the two paths have different security guarantees:

• Explicit mem_cube_id → _validate_cube_access is called (user-existence + cube-ACL check).
• Resolved-from-default path (None) → no _validate_cube_access call; the cube is trusted because it comes from get_user_cubes. This is likely safe, but worth making explicit in the docstring.

Suggestion: Either update get, update, and delete signatures to mem_cube_id: str | None (making the helper fully useful) or document the invariant clearly.

---

14. src/memos/mem_os/core.py (L1060-L1061)

Behavioral change for dump and load — two subtle differences introduced vs. the old code:

1. Falsy vs. None check: The old code used if not mem_cube_id (catching both None and ""). The new helper uses if mem_cube_id is None, so an empty-string mem_cube_id="" now goes down the _validate_cube_access path instead of falling back to the user's default cube. This is unlikely to matter in practice but is a silent behavioral difference.

2. New access-control enforcement: The old dump/load only fetched accessible cubes to resolve the default; they did not call _validate_cube_access for an explicitly supplied mem_cube_id. The new code unconditionally calls it. This is a correctness improvement (closes a missing authz check), but it is a breaking behavioral change for any caller (e.g., admin/system code) that was relying on dump/load bypassing the per-user ACL. Confirm this change is intentional and ensure it is covered by tests.

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (7/7 executed). memos_python_core/changed-python-source: 7/7. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-4a9d04ee70b50a44-20260808121109: 163/165 passed, 2 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: main

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (6/6 executed). memos_python_core/changed-repo-python: 6/6. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-4df9e12d917d9d25-20260808160035: 153/155 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: main