Repository navigation
fix: drop the duplicate _cleanup_semantic_history that shadows the guarded one - #1584
Conversation
test_get_version asserts that get_version() returns a version matching a hand-written pattern: release, optional .devN, optional local segment. On speedkick that assertion fails on every pull request, because the annotated tag v0.3.9-post1 (2026-08-31, "Post-release build from speedkick, after v0.3.9") sits on that branch's ancestry and setuptools-scm renders each build as 0.3.9.post2.devN+g<sha>, a shape the pattern never allowed. main passes today only because that tag is not yet reachable from it; it becomes reachable the moment speedkick merges, and any post-release tag would do the same. The test is deleted rather than patched. get_version() is a wrapper around importlib.metadata.version for two distribution names; nothing in it produces a version shape. The shape comes from setuptools-scm, the repository's tags and the working tree, so the test could only ever fail for environmental reasons, and that is its whole history: the regex was widened in #993 (dotted local segment), #1038 (two-component tag) and would have been again here, while the function under test has not changed since #951 wrote it. It also rejected a documented output: #951 specifies client_version as "not available" when memmachine-client is not installed, which is the case for a wheel install of the server, whose package does not depend on the client. The test passed only because the uv workspace installs every member. test_version_string stays; it tests the model's own string form. The check moves to the layer that owns it. The Test Server Package workflow already builds the wheels and installs them; a new step runs the installed memmachine-server --version and requires the reported server version to equal the version in the wheel's filename. That ties the binary to the artifact it shipped in without asserting anything about the version's format. Verified locally on this branch: the trimmed test file passes and is clean under ruff check and ruff format --check; the new step, run verbatim against wheels built from this tree and installed into a fresh virtualenv, passes (and reports "client: not available", which the deleted test would have rejected). Unrelated but worth knowing: the pytest workflow's test step has no shell key, so on windows it runs under pwsh, which does not stop on the first command's non-zero exit and returns the last command's status (the client suite). A server-suite failure on windows would not fail the job. On main the windows server run passes today (1830 passed on #1584's run), so nothing is hidden yet; on speedkick two windows-only failures are. Forcing bash there is left for when those are fixed. Same change as #1604 on speedkick, applied to main. Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Co-authored-by: Shu Wang <[email protected]>
marvinyu-memverge
left a comment
marvinyu-memverge
left a comment
There was a problem hiding this comment.
Approving - keeping the guarded one is the correct direction. Reviewed at 3baf7b5. Neither item below blocks the merge.
The bug is reachable today: get_semantic_service() raises ResourceNotReadyError whenever semantic memory is unconfigured (config auto-disables it when those fields are missing), and _delete_queued_session runs _delete_session_episode_store unconditionally. So on main, every session delete in a semantic-disabled deployment fails in the worker.
-
Could you add a test for this path? The existing
test_delete_episode_store_processes_in_batchesmonkeypatches_cleanup_semantic_historyout (with semantic memory disabled), which is exactly why it never caught the shadowing. Running the real method withget_semantic_serviceraisingResourceNotReadyError, and assertingdelete_episodesis called per batch and the session row gets deleted, would pin it. -
(Non-blocking) Consider gating on
self._conf.semantic_memory.enabledbefore callingget_semantic_service(), the waydelete_episodesalready does. Since the only producer of the not-ready error here is semantic memory being off, which is a normal config, the catch as written logs an ERROR with a traceback for every batch of every session delete in those deployments. The catch can stay as a fallback.
Observation: the PR body predates #1117 (async session deletion). On current main the failure doesn't abort delete_session - that call returns success; the background worker logs "Failed to delete session", leaves the episodes and the Deleted session row in place, and retries (and fails again) on each restart. Worth updating the description if it becomes the squash message.
Verified: the surviving definition is the guarded one and its only caller is _delete_session_episode_store; delete_episodes doesn't route through it. I also reproduced the F811 point - ruff check --select F811 passes on main's file with both definitions present.
|
Thanks for the fix, and sorry for the slow turnaround. It is approved but blocked by the repo's signed-commit rule: c6a637a is unsigned. Could you re-sign it and force-push? With The rebase drops the "Update branch" merge commit, which is fine: the re-signed commit lands on the same main tip with the same tree. Signing needs a key on your GitHub account (see CONTRIBUTING.md). If that is a hassle, say so or just leave it: if we have not heard back by Friday Oct 2, the commit will be re-signed from this account, keeping your authorship, message and sign-off unchanged, and merged. Written by Claude (Claude Code), posted from the account of the maintainer who commissioned it. |
…arded one `MemMachine` defines `_cleanup_semantic_history` twice (both added by MemMachine#1151). Python keeps the later one, which calls `self._resources.get_semantic_service()` with no guard, so the earlier definition -- the one written alongside the batched delete loop in `delete_session`, which catches `ResourceNotReadyError`, logs and returns -- is dead. As a result a `ResourceNotReadyError` escapes the loop before `episode_store.delete_episodes()` runs, and session deletion aborts with no episodes removed. Removing the unguarded duplicate restores the intended behaviour; it has no callers of its own. Signed-off-by: Tai An <[email protected]>
3baf7b5 to
39005db
Compare
|
Re-signed c6a637a as 39005db on top of current main, as announced above: your authorship, message and sign-off are unchanged, only the committer and signature differ (the 13-line deletion is byte-identical; the update-branch merge commit was dropped since the rebase sits on the main tip). If you push to this branch again, fetch first and reset to the new head. Thanks again for the fix. 🤖 Written by Claude Code (Claude Fable 5.1) on behalf of @edwinyyyu. |
3 participants
Problem
MemMachinedefines_cleanup_semantic_historytwice, both added by #1151:ResourceNotReadyErrorfromget_semantic_service(), logs, returnsdelete_session(_delete_episode_store, line 383)get_semantic_service()propagatesPython keeps the later definition, so the guarded one at 565 is dead code and line 383 silently gets the unguarded implementation.
That matters because the caller is a deletion loop:
If the semantic service is not ready,
ResourceNotReadyErrorescapes the loop beforedelete_episodes()runs, sodelete_sessionaborts with zero episodes deleted — the exact outcome the guard at 565 was written to prevent ("Semantic service not ready during history cleanup; skipping cleanup for episode IDs %s").ruff checkdoes not report this —F811is selected inpyproject.tomlbut does not fire on this pair, so lint gave no signal.Change
Remove the unguarded duplicate (−13 lines). No other file changes; the surviving definition also carries the more precise
list[EpisodeIdT]annotation.Verification
No services needed. Both definitions are lifted verbatim out of the file with
ast(by line range, so the real source is exercised, not a hand copy), bound into a class in the same order Python binds them, and the_delete_episode_storeloop is replayed against a_resourcesstub whoseget_semantic_service()raisesResourceNotReadyError:ruff format --checkis clean, andruff checkreports exactly the same findings before and after the patch (no new violations).If you would rather keep the simpler unguarded body and delete the guarded one instead, say so and I will flip the patch — but then
delete_sessionloses the error handling #1151 added for it.🤖 Generated with Claude Code