Dev v2.0.34

[bittergreen]

Description

Please include a summary of the change, the problem it solves, the implementation approach, and relevant context. List any dependencies required for this change.

Related Issue (Required): Fixes #issue_number

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Refactor (does not change functionality, e.g. code style improvements, linting)
• Documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

• Unit Test
• Test Script Or Test Steps (please provide)
• Pipeline Automated API Test (please provide)

Checklist

• I have performed a self-review of my own code | 我已自行检查了自己的代码
• I have commented my code in hard-to-understand areas | 我已在难以理解的地方对代码进行了注释
• I have added tests that prove my fix is effective or that my feature works | 我已添加测试以证明我的修复有效或功能正常
• I have created related documentation issue/PR in MemOS-Docs (if applicable) | 我已在 MemOS-Docs 中创建了相关的文档 issue/PR（如果适用）
• I have linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR（如果适用）
• I have mentioned the person who will review this PR | 我已提及将审查此 PR 的人

Reviewer Checklist

• closes #xxxx (Replace xxxx with the GitHub issue number)
• Made sure Checks passed
• Tests have been provided

[Memtensor-AI]

🤖 Open Code Review

Target: PR #2373
Task: b3961fb4fba95cff
Base: main
Head: dev-v2.0.34

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

⚠️ 1 warning(s) occurred during review.

---

1. src/memos/llms/openai.py (L14)

The name rate_limit now refers to two different things in this module: the imported sibling module (used as rate_limit.create_completion(...)) and the config attribute read as self.config.rate_limit. They appear within the same expressions at lines ~104 and ~126, e.g. rate_limit.create_completion(..., self.config.rate_limit). A reader scanning quickly can easily misread which rate_limit is being used, and a future local variable of the same name would silently shadow the module. Consider aliasing the import: from memos.llms import rate_limit as rl_module or reading the config into a local at the top of the method (rate_limit_config = self.config.rate_limit) to disambiguate.

---

2. src/memos/configs/llm_rate_limit.py (L120-L123)

The original Pydantic ValidationError is suppressed with from None, discarding the specific field value and constraint that failed. An operator who sets MEMSCHEDULER_REDIS_PORT=abc will only see Invalid LLM rate limit environment setting: redis_port with no indication of what value was rejected or why. Prefer from exc (or at minimum include the raw value in the message) so the cause is preserved and the error is actionable.

💡 Suggested Change

Before:

            except ValueError:
                raise ConfigurationError(
                    f"Invalid LLM rate limit environment setting: {name}"
                ) from None

After:

            except ValueError as exc:
                raise ConfigurationError(
                    f"Invalid LLM rate limit environment setting: {name}={raw!r}"
                ) from exc

---

3. src/memos/configs/llm_rate_limit.py (L69-L72)

The error message hard-codes the list of allowed fields rather than deriving it from _RULE_FIELDS. If _RULE_FIELDS is updated (a key added or removed), the message will silently fall out of sync with the actual enforcement. Reference _RULE_FIELDS directly so the message stays accurate automatically.

💡 Suggested Change

Before:

            if set(overrides) - _RULE_FIELDS:
                raise ValueError(
                    "rules only support qps, burst, max_wait_seconds, queue_capacity, retry_attempts"
                )

After:

            if set(overrides) - _RULE_FIELDS:
                raise ValueError(
                    f"rules only support: {', '.join(sorted(_RULE_FIELDS))}"
                )

---

4. src/memos/llms/rate_limit.py (L249-L253)

remaining_wait can become zero or negative before the next iteration. acquire() can consume nearly the full budget on a slow Redis round-trip, leaving nothing for subsequent attempts. A negative value means deadline = started + timeout is already in the past when acquire is entered on the next retry, so LLMRateLimitTimeoutError fires immediately at the first if now >= deadline check — the retry loop silently collapses and the caller gets a misleading timeout instead of the underlying API error.

Additionally, time.sleep(delay) at the end of the loop is not subtracted from remaining_wait, so retry backoff time is also not accounted for.

Suggestion: guard against exhaustion before re-entering acquire, and deduct sleep time too:

remaining_wait = rule.max_wait_seconds
for attempt in range(rule.retry_attempts + 1):
    if remaining_wait <= 0:
        raise LLMRateLimitTimeoutError("LLM permit waiting deadline exceeded")
    started = time.monotonic()
    limiter.acquire(timeout_seconds=remaining_wait)
    remaining_wait -= time.monotonic() - started
    ...
    time.sleep(delay)
    remaining_wait -= delay

---

5. src/memos/llms/rate_limit.py (L121-L127)

The Redis script call at lines 121–126 happens outside the self._condition lock, but the head check (self._queue[0] is waiter) on line 116 is done inside it. After the lock is released at line 120 (continue skips back to with self._condition), or after the inner with self._condition block exits to reach line 121, another thread whose waiter has just become head (because the current head advanced past next_check) can also exit its own with self._condition block and concurrently enter the Redis script call.

The class docstring states "only its head accesses Redis", but that invariant is not enforced during the Redis call itself — only at the moment of the check. Under contention this allows duplicate GCRA calls, over-consuming the burst budget.

To enforce the invariant, keep the Redis call inside a separate non-blocking mutex (e.g., a threading.Lock) held only by the current head, distinct from the condition variable used for waiting.

---

6. src/memos/llms/rate_limit.py (L187-L195)

config.redis_password is a plain str | None (not a SecretStr). Storing it as a dict key in the module-level _registry means the password is retained in a process-wide data structure for the lifetime of the process, where it is readily visible in heap dumps, gc.get_objects(), tracemalloc snapshots, and debug tooling.

Suggestion: use a hash of the password (e.g., hashlib.sha256) as the identity component, so the registry key never holds the raw credential:

import hashlib

def _hash_secret(value: str | None) -> str | None:
    if value is None:
        return None
    return hashlib.sha256(value.encode()).hexdigest()

identity = (
    config.redis_host,
    config.redis_port,
    config.redis_db,
    config.redis_username,
    _hash_secret(config.redis_password),
    config.redis_ssl,
    key,
)

---

7. src/memos/llms/rate_limit.py (L233-L238)

When the retry-after header contains a past HTTP date, requested_delay is negative. The >= 0 guard correctly skips it — but then falls through to the elif branch, which is False for APIStatusError, so execution continues to the exponential-backoff path (cap = min(...)). This means a stale or clock-skewed retry-after date silently substitutes a full exponential backoff instead of the server's intended minimal wait (or an immediate retry).

If the intent is "server said retry-after but the date is already past, retry immediately", the fix is to clamp negative values to zero before the guard:

if requested_delay is not None and math.isfinite(requested_delay):
    requested_delay = max(requested_delay, 0.0)
    return requested_delay if requested_delay <= rule.retry_max_delay else None

If the intent is "fall back to backoff when the header is unusable", add a comment explaining that, and consider whether returning None (no retry) is safer than silently over-delaying.

---

8. src/memos/llms/rate_limit.py (L252-L259)

time.monotonic() - started is evaluated twice: once to update remaining_wait and once inside the logger.info call. The log value is always slightly larger than what was subtracted from remaining_wait, so the two measurements are inconsistent — the logged permit_wait_ms does not reflect the actual time deducted from the budget. Capture the elapsed time once:

elapsed = time.monotonic() - started
remaining_wait -= elapsed
logger.info(
    "[LLM_RATE_LIMIT] sending model=%s attempt=%d permit_wait_ms=%.2f",
    model,
    attempt + 1,
    elapsed * 1000,
)

---

9. tests/llms/test_qps_rate_limit.py (L266-L267)

Redundant local import shadows the module-level from types import SimpleNamespace (line 9). The module-level import is still needed by test_permit_budget_is_shared_between_retries, so this local re-import is pure dead code. Remove the local import; the name resolves correctly from the module scope.

💡 Suggested Change

Before:

def test_closing_generator_closes_provider_stream(monkeypatch):
    from types import SimpleNamespace

After:

def test_closing_generator_closes_provider_stream(monkeypatch):

---

10. tests/llms/test_qps_rate_limit.py (L157-L158)

ConfigurationError is imported inline here while all other exception classes from the same module (LLMRateLimitError, LLMRateLimitQueueFullError, LLMRateLimitTimeoutError) are imported at module scope. Move this import to the top-level import block for consistency and so static analysis tools can see the full dependency graph.

💡 Suggested Change

Before:

def test_registry_rejects_conflicting_policy(monkeypatch):
    from memos.exceptions import ConfigurationError

After:

def test_registry_rejects_conflicting_policy(monkeypatch):

---

11. tests/llms/test_qps_rate_limit.py (L368-L374)

The test relies on the real time.sleep(0.02) taking longer than the 0.005 s acquire deadline, giving only a 4× timing margin. In rate_limit.py, the deadline check inside acquire happens after the Redis script returns, so the window that must elapse is the full round-trip through the mock (OS thread scheduling + the sleep itself). Under CI load this margin is frequently insufficient: if the calling thread is descheduled before it records started, or the mocked script is dispatched late, the deadline may expire vacuously without the timeout path being exercised, causing the pytest.raises block to fail non-deterministically. Use a much wider ratio (e.g. time.sleep(0.1) with acquire(0.005)) or, better, monkeypatch rate_limit.time.monotonic to return a value past the deadline immediately after the first script call, making the test deterministic.

💡 Suggested Change

Before:

def late(**_):
        time.sleep(0.02)
        return [1, 0]

    limiter = make_limiter(monkeypatch, late)
    with pytest.raises(LLMRateLimitTimeoutError):
        limiter.acquire(0.005)

After:

    def late(**_):
        time.sleep(0.1)   # 20× margin to survive CI scheduling jitter
        return [1, 0]

    limiter = make_limiter(monkeypatch, late)
    with pytest.raises(LLMRateLimitTimeoutError):
        limiter.acquire(0.005)

---

12. tests/llms/test_qps_rate_limit.py (L129-L133)

The busy-wait loop exits on either pending_count == 2 or deadline expiry, but the assert immediately after does not distinguish which branch was taken. On a loaded machine the second thread may not have enqueued itself before the 1-second deadline, causing a misleading AssertionError: assert 1 == 2 rather than an obvious timeout failure. Add an explicit timeout assertion before the count check so a slow-machine failure produces a clear message:

assert time.monotonic() < deadline, "timed out waiting for second thread to enqueue"
assert limiter.pending_count == 2

💡 Suggested Change

Before:

        deadline = time.monotonic() + 1
        while limiter.pending_count != 2 and time.monotonic() < deadline:
            time.sleep(0.001)
        try:
            assert limiter.pending_count == 2

After:

        deadline = time.monotonic() + 1
        while limiter.pending_count != 2 and time.monotonic() < deadline:
            time.sleep(0.001)
        try:
            assert time.monotonic() < deadline, "timed out waiting for second thread to enqueue"
            assert limiter.pending_count == 2

---

13. tests/configs/test_llm_rate_limit.py (L9)

python-dotenv is listed only under [tool.poetry.group.eval.dependencies] in pyproject.toml, not under [tool.poetry.group.test.dependencies]. Because this is a module-level import, every test in this file will fail at collection time with ModuleNotFoundError when the test suite is run without the eval extra (the normal CI path). Move python-dotenv to the test dependency group, or confine the import to the one function that needs it.

💡 Suggested Change

Before:

from dotenv import dotenv_values

After:

# In pyproject.toml [tool.poetry.group.test.dependencies], add:
# python-dotenv = "^1.1.1"

---

14. tests/configs/test_llm_rate_limit.py (L57)

MOS_CHAT_MODEL (the chat dialogue model) and MEMOS_LLM_RATE_LIMIT_RULES (the rate-limit policy) are independent configuration knobs. The assertion couples them: if the docker example ever updates one but not the other (e.g., adds a second model to RULES, or renames MOS_CHAT_MODEL), this test fails for reasons unrelated to the behaviour under test. Assert the actual expected rule key directly.

💡 Suggested Change

Before:

    assert set(config.rules) == {values["MOS_CHAT_MODEL"]}

After:

    assert set(config.rules) == {"gpt-4o-mini"}

---

15. tests/configs/test_llm_rate_limit.py (L39-L46)

rule_for can legitimately return None (the same file asserts this in other tests). Dereferencing rule.qps without a prior None-check means any regression in the wildcard/default-rule logic surfaces as an opaque AttributeError: 'NoneType' object has no attribute 'qps' rather than a clear test-failure message. Add an explicit assertion before the attribute access.

💡 Suggested Change

Before:

    rule = LLMRateLimitConfig.load().rule_for("gpt-4o-mini")
    assert (
        rule.qps,
        rule.burst,
        rule.max_wait_seconds,
        rule.queue_capacity,
        rule.retry_attempts,
    ) == (5, 2, 30, 16, 1)

After:

    rule = LLMRateLimitConfig.load().rule_for("gpt-4o-mini")
    assert rule is not None
    assert (
        rule.qps,
        rule.burst,
        rule.max_wait_seconds,
        rule.queue_capacity,
        rule.retry_attempts,
    ) == (5, 2, 30, 16, 1)

---

16. tests/configs/test_llm_rate_limit.py (L73-L74)

Same None-guard issue as test_default_rule_values: if rule_for returns None here (e.g., after a future change to the lookup logic), the failure is an AttributeError with no indication of which expectation broke. Assert non-None before accessing fields.

💡 Suggested Change

Before:

    assert (config.rule_for("gpt-4o-mini").qps, config.rule_for("gpt-4o-mini").burst) == (5, 2)
    assert (config.rule_for("other").qps, config.rule_for("other").burst) == (3, 1)

After:

    rule_mini = config.rule_for("gpt-4o-mini")
    rule_other = config.rule_for("other")
    assert rule_mini is not None
    assert rule_other is not None
    assert (rule_mini.qps, rule_mini.burst) == (5, 2)
    assert (rule_other.qps, rule_other.burst) == (3, 1)

---

🧹 Filtered 1 low-confidence OCR finding(s) before posting/fix-loop (existing_code_mismatch: 1).

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (72/72 executed, 5 skipped). memos_github_open_source/smoke: 1/1, memos_python_core/changed-repo-python: 71 passed, 5 skipped. Duration: 11s

Branch: dev-v2.0.34

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (72/72 executed, 5 skipped). memos_github_open_source/smoke: 1/1, memos_python_core/changed-repo-python: 71 passed, 5 skipped. Duration: 11s [advisory, non-gating] AI-generated tests on branch test/auto-gen-b3961fb4fba95cff-20260916145814: 19/41 passed, 22 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: dev-v2.0.34