Fix #2319: ### Pre-submission checklist - #2325
Conversation
…ation retries Failing skill crystallizations (LLM refusal, verifier mismatch, permanently unqualifying evidence) were retried on every skill tick because `runSkill` never persisted per-policy failure state and the documented `cooldownMs` config was never wired. A 25-day audit of a local deployment (issue MemTensor#2319) recorded 2,640 repeated failures across a small set of permanently-failing policies, each burning LLM budget and log volume. Add SQLite migration 019 with four additive columns on `policies` (attempts, backoff_until, last_attempt_at, last_failure_reason), a repo API for record/reset, an eligibility pre-gate that skips policies inside their exponential back-off window or in quarantine after max attempts, and matching failure-recording / success-reset hooks in `runSkill`. Back-off auto-invalidates when `policy.updatedAt > lastAttemptAt` so a fresh policy update always retries immediately (natural rehab). Defaults: 5 min base doubling to 24 h cap, quarantine after 8 attempts — turns the observed 2,640 retries into ~8 retries then zero, a >300× budget reduction on the worst-case permanent-failure policy. Verification: - npm run lint: clean (tsc --noEmit) - npm test: 1564 passed / 2 skipped across 183 files - 4 new eligibility tests + 3 new integration tests cover the back-off window, quarantine, auto-invalidation, and success reset Fixes MemTensor#2319
Memtensor-AI
added
ai:generated
🤖 Open Code ReviewTarget: PR #2325 ✅ OpenCodeReview: Review complete: 0 finding(s) across 11 selected item(s). Generated by cloud-assistant via Open Code Review. |
🔧 Open Code Review requested Agent fixOpen Code Review found 13 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
|
Nice work on the persistent back-off design — the auto-invalidation on One edge case we hit in production (and handled in #2326): The guard is one line in the skip branch: if (crystResult.skippedReason !== "llm-disabled") {
recordCrystallizationFailure(decision.policy.id, crystResult.skippedReason, deps);
}Plus a regression test that drives consecutive |
…indings Follow-up to the crystallization back-off change addressing 13 OCR findings on PR MemTensor#2325 (issue MemTensor#2319). Correctness / atomicity - policies.recordCrystallizationFailure now wraps the read-modify-write in a transaction so concurrent writers can no longer under-count attempts and slip past the quarantine threshold. - The same call now throws when the policy row is missing so a caller typo or race with deleteById surfaces as a logged warning (skill.backoff.record_failed) instead of silently succeeding. - resolveConfig cross-checks that crystallizationBackoffBaseMs <= crystallizationBackoffMaxMs; the individual schema ranges overlap (base up to 24h, max down to 1min) so nothing else catches inverted configs that would collapse the exponential curve into a flat retry. - Failure to reset back-off state after a successful crystallization is now surfaced as a warning and a skill.failed(stage=persist) bus event so operators have an observable signal instead of only a log line. Style / consistency - eligibility.ts: use strict !== null instead of != null (project rule) on the two nullable back-off comparisons. - skill.ts: align verify failure warnings.reason with the verify: prefix already persisted to crystallization_last_failure_reason so the bus event and the DB column agree. - policies.ts mapRow: cast crystallization_backoff_until / last_attempt_at directly to EpochMs instead of PolicyRow["updatedAt"] so future opaque-branding of updatedAt does not quietly change the field type. Docs / operability - eligibility.ts: expand the back-off gate comment to explain the ordering trade-off (quarantine reason dominates policy-status reason) so operators know to consult policy.status independently after clearing a quarantine. - migration 019: annotate the migrator bypass and add a partial index on crystallization_backoff_until (kept in sync with the SQL) so future dashboards / bulk-reset queries stay off a full table scan. - types.ts: tighten the crystallizationBackoff JSDoc so consumers know attempts is always >= 1 when the object is present (DB layer hides the attempts=0 clean state via mapRow). - schema.ts / skill/types.ts: cross-reference the new resolveConfig guard so operators know where invalid pairs are rejected. Tests - 4 new tests: config cross-field validation (accept + reject) and policies.recordCrystallizationFailure (missing-policy guard + monotonic increment through to quarantine). Verification - npm run lint: clean (tsc --noEmit) - npm test: 1568 passed / 2 skipped across 183 files
✅ Automated Test Results: PASSEDAll tests passed (21/21 executed). memos_local_plugin/unit: 21/21. Duration: 4s [advisory, non-gating] AI-generated tests on branch test/auto-gen-6694bfe346157994-20260902100927: 31/33 passed, 2 failed — these do NOT affect the PR verdict; review the branch manually. Branch: |
Memtensor-AI
added
status:ready
4 participants
Description
Fix for issue #2319 (skill crystallization retry storm) is implemented, tested, and pushed to the working branch.
The root cause was that
runSkillinapps/memos-local-plugin/core/skill/skill.tsrecorded no per-policy state on failure, and the documentedskill.cooldownMsconfig was dead code — so every subscriber trigger (l2.policy.induced,l2.policy.updated,reward.updated) re-attempted the same permanently-failing policies forever, producing the observed 2,640 repeated failures over 25 days.The fix adds a persistent per-policy back-off state to the
policiestable via additive migration 019 (four columns: attempts, backoff_until, last_attempt_at, last_failure_reason). Three newSkillConfigfields (crystallizationBackoffBaseMs,crystallizationBackoffMaxMs,crystallizationMaxAttempts, defaults 5 min / 24 h / 8 attempts) drive an exponential back-off.evaluateEligibilitygains a pre-gate that skips policies inside their back-off window or in quarantine, auto-invalidating wheneverpolicy.updatedAt > lastAttemptAtso that new evidence always retries immediately.runSkillrecords failures on all three branches (no-evidence, crystallize, verify) and resets state on successful upsert. Docs (README.md, ALGORITHMS.md §9) updated; the legacycooldownMsfield is preserved as a documented no-op for YAML compatibility.Verification:
npm run lintclean;npm testreports 1,564 passed / 2 skipped across 183 files with zero regressions. Four new eligibility tests and three new integration tests explicitly cover the back-off window, quarantine, auto-invalidation, and success reset — including a regression test that asserts the LLM is NOT re-invoked while back-off is active.Under the observed workload this turns 2,640 retries into ~8 then zero, a >300× budget reduction on the worst-case permanently-failing policy.
Related Issue (Required): Fixes #2319
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Not run; documentation-only change.
Checklist
@whipser030, @hijzy please review this PR.
Reviewer Checklist