fix: reward pipeline skips abandoned episodes (3 related fixes)#1784
Open
chiefmojo wants to merge 15 commits into
Open
fix: reward pipeline skips abandoned episodes (3 related fixes)#1784chiefmojo wants to merge 15 commits into
chiefmojo wants to merge 15 commits into
Conversation
Author
|
Added 4 additional fixes discovered during validation:
Verified across 3 hosts with DBs ranging from 462 to 30K traces. |
5 tasks
docs(memos-local-plugin): clarify install path and stale dir names (MemTensor#1540) The README's 'Quick start' section told users to use install.sh instead of npm install, but the warning was buried and users still tried 'npm install -g @memtensor/memos-local-plugin' first. The reporter in MemTensor#1540 encountered this on a Hermes deployment. This change: - Promotes the 'do not run npm install -g' notice to a prominent IMPORTANT callout explaining why global install is wrong (no agent-home deploy, no config.yaml, no bridge/viewer) and that the tarball intentionally ships built artifacts only. - Adds a Troubleshooting subsection covering the two specific symptoms in the bug report: the 'package not found' misread, and the stale web/ and site/ directory names (web/ is now viewer/, site/ was removed by commit 26e7e3d). - Mentions install.ps1 for Windows alongside install.sh. - CHANGELOG: record the docs fix and reference MemTensor#1540. Documentation-only change; no code or runtime behavior touched. Co-authored-by: MemOS AutoDev <autodev@memtensor.ai> Co-authored-by: Matthew <heimixiaozhuang@zju.edu.cn>
…_() got an unexpected keyword a (MemTensor#1889) fix: remove invalid chunker parameter from SystemParser test instantiation - SystemParser.__init__() signature changed to (embedder, llm=None) - Test was still passing chunker=None causing TypeError - Fixes all 5 failing tests in test_system_parser.py Fixes MemTensor#1888 Co-authored-by: MemOS AutoDev <autodev@memos.ai> Co-authored-by: Matthew <heimixiaozhuang@zju.edu.cn>
…tributeError when given None (MemTensor#1884) * test: add comprehensive tests for clean_json_response (issue MemTensor#1525) - Add test suite in tests/mem_os/test_format_utils.py - Cover None input ValueError with diagnostic message - Cover markdown removal, whitespace stripping, edge cases - Verify fix for AttributeError when LLM returns None * style: format clean_json_response tests --------- Co-authored-by: MemOS AutoDev <autodev@memos.ai> Co-authored-by: Matthew <heimixiaozhuang@zju.edu.cn>
…date_cube_access — fails for ev (MemTensor#1903) fix: validate current user not target in share_cube_with_user (MemTensor#1901) share_cube_with_user(cube_id, target_user_id) called _validate_cube_access(cube_id, target_user_id), but the validator signature is (user_id, cube_id). The cube_id therefore landed in the user_id slot and _validate_user_exists raised "User '<cube_id>' does not exist or is inactive" for every well-formed call, making the API unusable. The in-code comment "Validate current user has access to this cube" already documented the correct intent: the sharing user (self.user_id) must have access to the cube being shared, not the target. Switch the call to self._validate_cube_access(self.user_id, cube_id). The target user's existence is independently checked on the next line via validate_user(target_user_id), so that path is unchanged. Add regression tests in tests/mem_os/test_memos_core.py that pin down: - validate_user_cube_access is consulted with (self.user_id, cube_id), - add_user_to_cube is called with (target_user_id, cube_id) on success, - a missing target raises "Target user '<id>' does not exist". Closes MemTensor#1901 Co-authored-by: MemOS AutoDev Bot <autodev@memtensor.local> Co-authored-by: Matthew <heimixiaozhuang@zju.edu.cn>
- episodeRewardIsDirty() now includes closeReason=abandoned (219 of 224 closed episodes were silently skipped) - Added 10-min setInterval for autoRescoreDirtyClosedEpisodes() so the daemon bridge doesn"t go permanently idle after bootstrap
ensure_viewer_daemon() probes port 18800 with a 15-second timeout. When core.init() rescores dirty episodes it can take minutes, keeping the port unbound past the deadline. ensure_viewer_daemon() then gives up, releases the startup lock, and the next keepalive cycle spawns a replacement daemon that kills the in-progress one — creating a restart loop that interrupts scoring mid-batch. Fix: in daemon mode, bind the HTTP server first so the health probe succeeds within seconds, then run core.init() asynchronously in the background. Non-daemon (stdio/JSON-RPC) mode is unchanged — it still runs init synchronously so host-LLM fallback is available during recovery. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ardIsDirty() reward.skipped=true was being set on abandoned episodes when the reward runner decided the conversation was too short (< 2 turns). This flag then permanently excluded them from recovery rescoring — episodeRewardIsDirty() returned false before the closeReason==="abandoned" check was reached, leaving 173 episodes stuck unscored indefinitely. Fix: only honor reward.skipped for episodes that are NOT (abandoned + no prior recovery attempt). recoverDirtyClosedEpisodes() patches closeReason → "finalized" after processing, so if reward still skips on recovery the next dirty check sees closeReason !== "abandoned" and stops retrying — exactly one recovery pass, no infinite loop. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lagged episodes Episodes tagged lightweightMemory:true during a prior session when lightweight mode was active were permanently excluded from scoring even after the config was changed to enabled:false. Three guards enforced this unconditionally: the pre-filter in autoRescoreDirtyClosedEpisodes and init(), the skip inside recoverDirtyClosedEpisodes, and the check in the capture subscriber. All three now condition on the *current* handle.algorithm.lightweightMemory.enabled value. The snapshot emitted for a legacy-flagged episode also has the lightweightMemory field stripped before the event fires, so the capture subscriber receives a clean snapshot. When lightweight mode is on, behavior is unchanged. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Commit a054c9b8 introduced two bugs when rebuilding the FTS after dedup:
1. Renamed FTS column from trace_id to id, breaking the JOIN in traces.ts
2. Used FTS5 special 'delete' command in the UPDATE trigger, which only
works for external-content tables — throws "SQL logic error" on regular
FTS5 tables, causing every updateScore() call to fail silently
Migration 013 drops the broken table + triggers and rebuilds with the
canonical trace_id column and correct direct-DELETE trigger syntax.
Applied to Faye's DB manually; Dora and Violet will auto-apply on restart.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The triviality-gate skip path wrote reward.skipped=true but never cleared rewardDirty from meta_json. Since episodeRewardIsDirty() checks the rewardDirty object flag before the skip gate, skipped episodes with the flag set would re-enter the dirty scan on every bridge restart, scoring and re-skipping indefinitely. Normal scoring path already had rewardDirty: undefined — this mirrors that pattern in the skip branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The skip path wrote reward.skipped=true but never called setRTask(), leaving r_task=NULL. For abandoned episodes episodeRewardIsDirty() falls through to the r_task==null check and returns true, causing those episodes to re-enter the dirty scan on every bridge start, get re-skipped, and loop indefinitely. Setting r_task=0 before updateMeta means the null-r_task branch in episodeRewardIsDirty() no longer fires, permanently clearing the episode from the dirty scan after its first skip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
clampLimit() in _helpers.ts caps list() at 500 regardless of the limit argument passed. The prior limit:1000 change (87165daf) was a no-op — both values hit the same ceiling, leaving episodes beyond rank 500 permanently invisible to the dirty scan. Replace both scan sites (startup + periodic) with collectDirtyClosedEpisodes(), which paginates in 500-row pages until exhausted. All closed episodes are now covered regardless of total count. This was also the root cause of the "dirty-17" mystery: those episodes were at ranks 536-924, outside the 500-row window. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Skipped episodes (triviality gate) wrote reward.skipped=true but no traceCount, making them appear dirty to the consistency tools. traceIds is already in scope at the skip check — write traceCount: traceIds.length so all closed episodes carry consistent metadata. No behaviour change for the bridge: episodeRewardIsDirty condition 3 returns false for finalized-skipped episodes before condition 5 is evaluated, so adding traceCount does not affect rescore decisions.
…mode recoverDirtyClosedEpisodes relied on flush() → reward.drain() to fire R_human scoring after the capture pass. flush() returns early in lightweight mode (the default), so the reward subscriber's 30 s timer was cancelled by shutdown() before it fired — leaving traceCount permanently mismatched and the episode dirty on every restart. Fix: after flush() drains the capture pass, explicitly call rewardRunner.run() for any episode that episodeRewardIsDirty() still considers dirty — mirroring the pattern already used by recoverOpenEpisodesAsSessionEnd. A second flush() then drains downstream (L2 / L3 / skills). Regression test: dirty-reward recovery does not insert orphan traces — seeded episode with traceCount=1 and 2 trace IDs (one having a tool call whose endedAt differs from the trace ts, which produces an orphan step in runReflect). Verifies that: 1. trace_ids_json stays at 2 after recovery (orphan insert guard). 2. traceCount is updated to 2 after the first recovery pass. 3. A second restart does not re-score the episode (loop stopped). Also fixes the pre-existing test "rescoring closed episodes when traces were appended after the last reward" which failed for the same reason. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…de-scoring # Conflicts: # apps/memos-local-plugin/bridge.cts # apps/memos-local-plugin/core/pipeline/memory-core.ts # apps/memos-local-plugin/tests/unit/pipeline/memory-core.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The reward scoring pipeline processes only ~7 episodes on bootstrap and then goes permanently idle, leaving the vast majority of traces unscored. On a 43 MB database with 3,600 traces, only 45 (1.3%) had r_human scores before fixing.
Root Cause & Fixes
Three interacting bugs, all in
apps/memos-local-plugin/:Fix 1:
episodeRewardIsDirty()excluded abandoned episodesThe dirty-check condition only matched
closeReason === "finalized"orrecoveryReason === "missed_session_end". 219 of 224 closed episodes hadcloseReason: "abandoned".Fix: Add
closeReason === "abandoned"to the rescore condition + a 10-minute periodic rescore timer.Fix 2: Daemon HTTP server must bind before
core.init()The daemon bridge started
core.init()beforestartHttpServer(). When init rescores dirty episodes (5+ minutes of LLM calls), port 18800 stays free. The Python watchdog times out its 15-second health probe and spawns a new daemon — killing the in-progress one.Fix: In daemon mode, bind HTTP server first, then run init asynchronously.
Fix 3:
reward.skippedblocked abandoned episodes from recoveryepisodeRewardIsDirty()checkedreward.skipped === trueBEFOREcloseReason === "abandoned". The reward runner correctly skipped 173 one-turn episodes in a previous session, but that flag permanently excluded them from recovery.Fix:
reward.skippedis only honored if the episode is NOT (abandoned + no prior recovery). Abandoned episodes get one pass; after that, closeReason is patched to "finalized" and the normal skip guard works.Verification