Skip to content

fix(plugin): archive idle low-eta skills - #2209

Open
CovD831 wants to merge 9 commits into
MemTensor:dev-v2.0.29from
CovD831:codex/fix-skill-idle-archive
Open

fix(plugin): archive idle low-eta skills#2209
CovD831 wants to merge 9 commits into
MemTensor:dev-v2.0.29from
CovD831:codex/fix-skill-idle-archive

Conversation

@CovD831

@CovD831 CovD831 commented Aug 4, 2026

Copy link
Copy Markdown

Description

Archive active Skills that remain below the retrieval ETA threshold after a configurable period of retrieval inactivity.

This change:

  • adds algorithm.skill.idleArchiveMs (30 days by default);
  • uses lastUsedAt ?? createdAt as the idle baseline;
  • queries eligible candidates directly in SQLite and drains 500-row batches oldest-first;
  • runs the scan from the existing lifecycle tick without adding a timer;
  • preserves the public four-argument shouldArchiveIdle API;
  • emits structured lifecycle status events/logs and documents the behavior.

No new dependencies.

Related Issue (Required): Fixes #2144

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Documentation update

How Has This Been Tested?

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

Commands and results:

  • Focused lifecycle/storage/config/OpenClaw integration suite: 67/67 passed.
  • Broader skill/config/storage/pipeline/OpenClaw suite: 257/257 passed.
  • Full unit suite: 1302 passed, 1 skipped. Two repository-layout tests failed only because the remote test copy was outside the normal apps/memos-local-plugin layout and could not resolve repository-level workflow files.
  • tsc --noEmit: passed.
  • tsc -p tsconfig.build.json: passed.
  • git diff --check: passed.

Additional verification:

  • a 501-candidate backlog test verifies that one lifecycle tick drains multiple batches;
  • mutation checks verify the suite catches wrong idle baselines, an inclusive ETA boundary, disabled lifecycle invocation, single-batch starvation, and missing recordUse persistence.

make format could not start locally because Poetry is unavailable. This change is confined to the TypeScript plugin and does not modify Python files.

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 | 我已添加测试以证明我的修复有效或功能正常
  • MemOS-Docs issue/PR is not applicable; plugin-local documentation is updated
  • I have linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR(如果适用)
  • I have mentioned the person who will review this PR | Maintainer assignment requested after submission

Reviewer Checklist

@Memtensor-AI Memtensor-AI added area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Aug 4, 2026
@Memtensor-AI

Memtensor-AI commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2209
Task: 54ae872c8776d8d7
Base: dev-v2.0.29
Head: codex/fix-skill-idle-archive

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


1. apps/memos-local-plugin/core/skill/subscriber.ts (L217-L220)

The lifecycleTick function has no error handling. Any exception from the repo calls (listIdleArchiveCandidates, archiveIdleBatch) or deps.bus.emit will propagate as an unhandled rejection out of the scheduled tick. Unlike drain(), which wraps its work in a try/catch and logs the error, lifecycleTick has no equivalent guard. The caller site in the scheduler does not add one either.

Add a try/catch so a single failing tick is logged and isolated rather than potentially crashing the scheduler or silently killing future ticks.

💡 Suggested Change

Before:

  /** Promote eligible candidates and archive stale low-η active skills. */
  async function lifecycleTick(): Promise<void> {
    const at = nowMs();
    const candidates = deps.repos.skills.list({ status: "candidate", limit: 500 });

After:

  /** Promote eligible candidates and archive stale low-η active skills. */
  async function lifecycleTick(): Promise<void> {
    try {
      await _lifecycleTickInner();
    } catch (err) {
      log.error("skill.lifecycle_tick.failed", {
        err: err instanceof Error ? err.message : String(err),
      });
    }
  }

  async function _lifecycleTickInner(): Promise<void> {
    const at = nowMs();
    const candidates = deps.repos.skills.list({ status: "candidate", limit: 500 });

2. apps/memos-local-plugin/core/skill/subscriber.ts (L286-L293)

The skill.idle_archive_batch_limit_reached warning is unreachable. The while loop condition is batchesProcessed < IDLE_ARCHIVE_MAX_BATCHES_PER_TICK, so when batchesProcessed reaches IDLE_ARCHIVE_MAX_BATCHES_PER_TICK the loop exits at the top-of-loop check before the body runs again — the inner if (batchesProcessed === IDLE_ARCHIVE_MAX_BATCHES_PER_TICK) block never executes. Additionally, the preceding if (archiveCandidates.length < IDLE_ARCHIVE_BATCH_LIMIT) break would have already exited the loop in any realistic scenario where the final batch is partial.

Move the limit-reached warning to after the while-loop, conditioned on whether the loop actually exhausted its budget:

💡 Suggested Change

Before:

      if (archiveCandidates.length < IDLE_ARCHIVE_BATCH_LIMIT) break;
      if (batchesProcessed === IDLE_ARCHIVE_MAX_BATCHES_PER_TICK) {
        log.warn("skill.idle_archive_batch_limit_reached", {
          batchCount: batchesProcessed,
          archivedCount: archivedTotal,
          batchSize: IDLE_ARCHIVE_BATCH_LIMIT,
        });
      }

After:

      if (archiveCandidates.length < IDLE_ARCHIVE_BATCH_LIMIT) break;
    }
    if (batchesProcessed >= IDLE_ARCHIVE_MAX_BATCHES_PER_TICK) {
      log.warn("skill.idle_archive_batch_limit_reached", {
        batchCount: batchesProcessed,
        archivedCount: archivedTotal,
        batchSize: IDLE_ARCHIVE_BATCH_LIMIT,
      });
    }

3. apps/memos-local-plugin/core/skill/subscriber.ts (L275-L285)

A persistent full-batch stall (all candidates invalidated by concurrent writers, archiveCandidates.length === IDLE_ARCHIVE_BATCH_LIMIT, archivedThisBatch === 0) takes the continue path, which increments batchesProcessed without making any progress. If the stall holds across multiple iterations, the entire per-tick budget (IDLE_ARCHIVE_MAX_BATCHES_PER_TICK) is consumed doing nothing, and genuine archival candidates on later pages are starved until the next tick.

Track consecutive stall count and break early once a threshold is exceeded, rather than burning the full budget:

💡 Suggested Change

Before:

      if (archiveCandidates.length > 0 && archivedThisBatch === 0) {
        // A full zero-change batch was invalidated by concurrent writers.
        // Re-query so later eligible rows are not abandoned for this tick.
        log.warn("skill.idle_archive_stalled", {
          candidateCount: archiveCandidates.length,
          cutoff,
          minEtaForRetrieval: deps.config.minEtaForRetrieval,
        });
        if (archiveCandidates.length < IDLE_ARCHIVE_BATCH_LIMIT) break;
        continue;
      }

After:

      if (archiveCandidates.length > 0 && archivedThisBatch === 0) {
        log.warn("skill.idle_archive_stalled", {
          candidateCount: archiveCandidates.length,
          cutoff,
          minEtaForRetrieval: deps.config.minEtaForRetrieval,
        });
        // Break regardless of batch fullness — a zero-progress batch means
        // concurrent writers have invalidated all candidates; continuing
        // would only burn the remaining per-tick budget with no benefit.
        break;
      }

4. apps/memos-local-plugin/core/skill/subscriber.ts (L40-L41)

The comment encodes the product 10 × 500 = 5,000 as a literal number. If IDLE_ARCHIVE_BATCH_LIMIT is changed in skills.ts, the comment silently becomes wrong with no compile-time signal. Reference the imported constant in the comment instead, or derive the cap at runtime:

💡 Suggested Change

Before:

/** Bound one lifecycle pass to 5,000 archival writes. */
const IDLE_ARCHIVE_MAX_BATCHES_PER_TICK = 10;

After:

/** Bound one lifecycle pass to {@link IDLE_ARCHIVE_MAX_BATCHES_PER_TICK} × {@link IDLE_ARCHIVE_BATCH_LIMIT} archival writes. */
const IDLE_ARCHIVE_MAX_BATCHES_PER_TICK = 10;

5. apps/memos-local-plugin/core/storage/repos/skills.ts (L192-L198)

Math.max(1, NaN) returns NaN in JavaScript, not 1. If input.limit is NaN (e.g., the result of a failed numeric conversion upstream), Math.floor(NaN) produces NaN, Math.min(500, NaN) produces NaN, and Math.max(1, NaN) also produces NaN. That NaN is then bound to LIMIT @limit in SQLite, which silently ignores an invalid LIMIT and may return all matching rows — exactly the opposite of what the clamp is trying to prevent.

Fix by guarding before the arithmetic:

const rawLimit = Number.isFinite(input.limit) ? input.limit! : IDLE_ARCHIVE_BATCH_LIMIT;
limit: Math.max(1, Math.min(IDLE_ARCHIVE_BATCH_LIMIT, Math.floor(rawLimit))),
💡 Suggested Change

Before:

        limit: Math.max(
          1,
          Math.min(
            IDLE_ARCHIVE_BATCH_LIMIT,
            Math.floor(input.limit ?? IDLE_ARCHIVE_BATCH_LIMIT),
          ),
        ),

After:

        limit: Math.max(
          1,
          Math.min(
            IDLE_ARCHIVE_BATCH_LIMIT,
            Math.floor(Number.isFinite(input.limit) ? input.limit! : IDLE_ARCHIVE_BATCH_LIMIT),
          ),
        ),

6. apps/memos-local-plugin/core/storage/repos/skills.ts (L106-L110)

The method accepts an unbounded ids array with no internal enforcement of IDLE_ARCHIVE_BATCH_LIMIT. The current sole caller always passes a pre-sized slice, but the public signature makes this a silent contract. A future caller passing an oversized array will hold the SQLite write lock for the full duration of potentially thousands of individual UPDATEs with no warning or truncation.

Consider asserting or slicing at the boundary:

if (ids.length > IDLE_ARCHIVE_BATCH_LIMIT) {
  throw new Error(`archiveIdleBatch: ids.length ${ids.length} exceeds IDLE_ARCHIVE_BATCH_LIMIT ${IDLE_ARCHIVE_BATCH_LIMIT}`);
}
💡 Suggested Change

Before:

    archiveIdleBatch(
      ids: readonly SkillId[],
      input: { minEtaForRetrieval: number; cutoff: number; updatedAt: number },
    ): SkillId[] {
      if (ids.length === 0) return [];

After:

    archiveIdleBatch(
      ids: readonly SkillId[],
      input: { minEtaForRetrieval: number; cutoff: number; updatedAt: number },
    ): SkillId[] {
      if (ids.length === 0) return [];
      if (ids.length > IDLE_ARCHIVE_BATCH_LIMIT) {
        throw new Error(
          `archiveIdleBatch: ids.length ${ids.length} exceeds IDLE_ARCHIVE_BATCH_LIMIT ${IDLE_ARCHIVE_BATCH_LIMIT}`,
        );
      }

7. apps/memos-local-plugin/core/storage/repos/skills.ts (L179-L183)

The comment is slightly misleading. It's the WHERE clause that excludes non-idle skills, not the filtering that "prevents starvation" — starvation is prevented by the ORDER BY COALESCE(last_used_at, created_at) ASC ensuring oldest-idle-first ordering. Consider revising to make the intent of each clause clear:

 * Return one oldest-first batch of active skills that already satisfy
 * the idle-archive predicate. The WHERE clause excludes recently-active
 * skills; ORDER BY ensures the longest-idle candidates are processed first
 * so no eligible skill is indefinitely skipped.

Generated by cloud-assistant via Open Code Review.

@CovD831

CovD831 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Addressed the actionable Open Code Review findings in c5c0d0c:

  • Enforced a one-hour minimum for idleArchiveMs, with boundary tests and documentation updates, so 0 cannot trigger immediate bulk archival.
  • Renamed the inner list to archiveCandidates.
  • Shared IDLE_ARCHIVE_BATCH_LIMIT between the repository and subscriber.
  • Reordered the zero-progress and partial-batch exits so any non-empty stalled batch is logged consistently.

I did not add cursor pagination for finding 3: the repository query already returns only rows satisfying the archive predicate, and synchronous setStatus removes processed rows from the next active-only query. The existing zero-progress guard still bounds unexpected repository behavior.

I also retained the direct COALESCE expressions for finding 6. They are trivial, and a SQLite CTE may be inlined without reducing evaluation while making the query less direct.

Verification:

  • focused lifecycle/storage/config/OpenClaw suite: 67/67 passed
  • broader related suite: 331/331 passed
  • tsc --noEmit: passed
  • production TypeScript build: passed

@MatthewZhuang
MatthewZhuang changed the base branch from main to dev-v2.0.29 August 4, 2026 15:16
@CovD831

CovD831 commented Aug 4, 2026

Copy link
Copy Markdown
Author

The PR was retargeted from main to dev-v2.0.29 after the review update. I aligned the branch in 243b0a6 without rewriting published history and removed the unrelated #2208 delta. The final diff against the new base remains scoped to the 16 idle-archive files (392 additions, 10 deletions).

Fresh verification from the new base composition:

  • focused suite: 67/67 passed
  • broader related suite: 329/329 passed
  • TypeScript typecheck and production build: passed

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (67/67 executed). memos_local_plugin/unit: 67/67. Duration: 9s [advisory, non-gating] AI-generated tests on branch test/auto-gen-83c5b20bcb739226-20260804233503: 40/40 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: codex/fix-skill-idle-archive

@Memtensor-AI Memtensor-AI added status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 and removed status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Aug 4, 2026
@Memtensor-AI Memtensor-AI added status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 and removed status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 labels Aug 4, 2026
@CovD831

CovD831 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Addressed the latest Open Code Review finding in 0ff0a72.

The idle-archive loop now processes at most 10 batches per lifecycle tick (5,000 Skills at the existing 500-row batch size). Reaching the cap emits skill.idle_archive_batch_limit_reached with batch and archive counts; any remaining eligible Skills stay active and are processed by the next lifecycle tick.

The regression test first demonstrated the old behavior by archiving all 5,001 rows, then verifies the bounded behavior: 5,000 archived and 1 deferred after the first tick, followed by all 5,001 archived after the second tick.

Verification:

  • focused lifecycle/storage/config/OpenClaw suite: 68/68 passed
  • broader related suite: 330/330 passed
  • TypeScript typecheck and production build: passed

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (68/68 executed). memos_local_plugin/unit: 68/68. Duration: 18s [advisory, non-gating] AI-generated tests on branch test/auto-gen-d48df831a127afae-20260805003545: 56/56 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: codex/fix-skill-idle-archive

@Memtensor-AI Memtensor-AI added status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 and removed status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Aug 4, 2026
@Memtensor-AI Memtensor-AI added status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 and removed status:ready Ready for implementation; waiting for assignee or AI dispatch | 可进入实现,等待认领或派发 labels Aug 6, 2026
@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (68/68 executed). memos_local_plugin/unit: 68/68. Duration: 18s [advisory, non-gating] AI-generated tests on branch test/auto-gen-6f2c3f5ab4b293a2-20260806234138: 64/64 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: codex/fix-skill-idle-archive

@syzsunshine219

Copy link
Copy Markdown
Collaborator

Addressed the actionable concurrency findings in 0dad4af3:

  • Idle archival now uses one conditional SQLite update that re-checks status, ETA, and idle cutoff atomically. Events are emitted only when the active-to-archived transition was actually applied.
  • A full batch invalidated by concurrent writers now continues to later candidates instead of abandoning the remaining pages. Partial final batches still terminate normally.
  • Added repository and lifecycle regressions for concurrent recent-use/status changes and a 501-candidate full-batch race.

Verification on the final branch composition:

  • focused lifecycle/storage suite: 69/69 passed
  • full unit suite: 1306 passed, 1 skipped
  • integration suite: 5/5 passed
  • lint, TypeScript build, and git diff --check: passed

The current GitHub Actions red jobs are failing during Set up job before repository code executes; I am waiting for the matrix to finish and will rerun only failed jobs.

@syzsunshine219

Copy link
Copy Markdown
Collaborator

Follow-up 10cda282 addresses the actionable performance/maintenance findings from the latest review:

  • archives each 500-row batch in one SQLite transaction and returns only IDs whose conditional transition applied
  • shares the authoritative SQL predicate between candidate selection and conditional update
  • removes the redundant in-memory predicate check
  • adds a forced mid-batch failure regression proving the transaction rolls back the earlier update

I reviewed the remaining OCR advisories. The batch-limit warning is reachable inside the 10th loop body and is covered by the 5,001-row regression. A full zero-change batch means concurrent writers invalidated the conditional updates; re-querying a full page is intentional so later valid candidates are not abandoned (the 501-row concurrency regression covers this). Lifecycle errors continue to propagate to the orchestrator instead of making a failed flush appear successful.

Final local verification: 18/18 focused tests, 1306 unit tests passed with 1 skipped, 5/5 integration tests, TypeScript lint, production build, and git diff --check.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

✅ Automated Test Results: PASSED

All tests passed (69/69 executed). memos_local_plugin/unit: 69/69. Duration: 17s [advisory, non-gating] AI-generated tests on branch test/auto-gen-54ae872c8776d8d7-20260807004733: 1/84 passed, 83 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: codex/fix-skill-idle-archive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:plugin OpenClaw & Hermes status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

memos-local-plugin: 技能归档(archived)状态从未被触发

4 participants