feat(learning): schedule consolidation maintenance#214
Merged
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Review Summary by Qodo
WalkthroughsDescription• Introduce LearningMaintenanceScheduler with time, session-count, size, and idle triggers • Move consolidation, mining, and trend detection from session teardown to scheduler pipeline • Expose backing-file size from AutoMemoryStore via new largestBackingFileBytes() method • Add comprehensive test coverage for scheduler trigger behavior Diagramflowchart LR
A["Session Close Event"] -->|onSessionClosed| B["LearningMaintenanceScheduler"]
C["Scheduler Tick"] -->|time/size/idle checks| B
B -->|triggers maintenance| D["runLearningMaintenancePipeline"]
D -->|consolidate| E["MemoryConsolidator"]
D -->|mine patterns| F["CrossSessionPatternMiner"]
D -->|detect trends| G["TrendDetector"]
H["AutoMemoryStore"] -->|largestBackingFileBytes| B
File Changes1. aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java
|
Code Review by Qodo
1. memory-consolidation CronJob missing
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a configurable LearningMaintenanceScheduler and integrates it into AceClawDaemon to centralize and schedule deferred memory consolidation, cross-session pattern mining, and trend detection; AutoMemoryStore can report largest backing file size; scheduler unit tests and documentation updates included. Changes
Sequence DiagramsequenceDiagram
participant Daemon as AceClawDaemon
participant Scheduler as LearningMaintenanceScheduler
participant Executor as ScheduledExecutor
participant Pipeline as MaintenancePipeline
participant Memory as AutoMemoryStore
participant Miners as CrossSession/TrendDetector
Daemon->>Scheduler: start()
Scheduler->>Executor: start tick loop
Executor->>Scheduler: tick()
Scheduler->>Scheduler: evaluate triggers (TIME, SIZE, IDLE)
alt trigger condition met
Scheduler->>Pipeline: run(trigger) -- virtual thread
Pipeline->>Memory: consolidateMemory()
Pipeline->>Miners: mineCrossSessionPatterns()
Pipeline->>Miners: detectTrends()
Pipeline->>Memory: writeDailyJournal()
end
Daemon->>Scheduler: onSessionClosed()
Scheduler->>Scheduler: increment session count
alt sessionCount ≥ threshold
Scheduler->>Pipeline: run("session-count")
Pipeline->>Memory: consolidateMemory()
Pipeline->>Miners: mine + detect
end
Daemon->>Scheduler: stop()
Scheduler->>Executor: shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1607
to
+1617
| if (learningMaintenanceScheduler != null) { | ||
| try { | ||
| learningMaintenanceScheduler.start(); | ||
| shutdownManager.register(new ShutdownManager.ShutdownParticipant() { | ||
| @Override public String name() { return "Learning Maintenance Scheduler"; } | ||
| @Override public int priority() { return 87; } | ||
| @Override public void onShutdown() { learningMaintenanceScheduler.stop(); } | ||
| }); | ||
| } catch (Exception e) { | ||
| log.error("Learning maintenance scheduler startup failed (daemon will continue): {}", e.getMessage(), e); | ||
| } |
There was a problem hiding this comment.
1. memory-consolidation cronjob missing 📎 Requirement gap ⛯ Reliability
Daemon startup starts LearningMaintenanceScheduler but does not register a CronJob named memory-consolidation with CronScheduler. As a result, the required 6-hour CronScheduler-based schedule is not configured.
Agent Prompt
## Issue description
Daemon startup does not register a `CronJob` named `memory-consolidation` with `CronScheduler`, and the 6-hour schedule is implemented via an internal `ScheduledExecutorService` instead of `CronScheduler`.
## Issue Context
Compliance requires the daemon to automatically register a `CronScheduler` job named `memory-consolidation` during startup, and that job must execute every 6 hours.
## Fix Focus Areas
- aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java[1607-1618]
- aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningMaintenanceScheduler.java[59-77]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+1774
to
+1828
| private void runLearningMaintenancePipeline( | ||
| String trigger, | ||
| AutoMemoryStore memoryStore, | ||
| Path archiveDir, | ||
| DailyJournal journal, | ||
| CrossSessionPatternMiner crossSessionPatternMiner, | ||
| TrendDetector trendDetector, | ||
| String workspaceHash, | ||
| Path workingDir | ||
| ) { | ||
| try { | ||
| var result = MemoryConsolidator.consolidate(memoryStore, workingDir, archiveDir); | ||
| if (result.hasChanges() && journal != null) { | ||
| journal.append("Memory consolidated (" + trigger + "): " | ||
| + result.deduped() + " deduped, " | ||
| + result.merged() + " merged, " | ||
| + result.pruned() + " pruned"); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Memory consolidation failed (trigger={}): {}", trigger, e.getMessage()); | ||
| } | ||
|
|
||
| if (crossSessionPatternMiner != null && historicalLogIndex != null) { | ||
| try { | ||
| var mined = crossSessionPatternMiner.mine( | ||
| historicalLogIndex, memoryStore, workspaceHash, workingDir); | ||
| if ((!mined.frequentErrorChains().isEmpty() | ||
| || !mined.stableWorkflows().isEmpty() | ||
| || !mined.convergingStrategies().isEmpty() | ||
| || !mined.degradationSignals().isEmpty()) | ||
| && journal != null) { | ||
| journal.append("Cross-session miner (" + trigger + "): " | ||
| + mined.frequentErrorChains().size() + " error chains, " | ||
| + mined.stableWorkflows().size() + " stable workflows, " | ||
| + mined.convergingStrategies().size() + " converging strategies, " | ||
| + mined.degradationSignals().size() + " degradation signals"); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Cross-session pattern mining failed (trigger={}): {}", trigger, e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| if (trendDetector != null && historicalLogIndex != null) { | ||
| try { | ||
| var trends = trendDetector.detect( | ||
| historicalLogIndex, memoryStore, workspaceHash, workingDir); | ||
| if (!trends.isEmpty() && journal != null) { | ||
| journal.append("Trend detector (" + trigger + "): " + trends.size() | ||
| + " significant trends. " + summarizeTrends(trends)); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Trend detection failed (trigger={}): {}", trigger, e.getMessage()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
2. runlearningmaintenancepipeline missing stages 📎 Requirement gap ✓ Correctness
The new maintenance pipeline runs consolidation, cross-session mining, and trend detection only, omitting SessionAnalyzer, stale HistoricalLogIndex rebuild, and feeding results into AutoMemoryStore + SkillProposalEngine. This breaks the required end-to-end retrospective learning sequence.
Agent Prompt
## Issue description
`runLearningMaintenancePipeline` omits required stages from the retrospective learning pipeline (SessionAnalyzer, conditional HistoricalLogIndex rebuild, and feeding outputs to AutoMemoryStore + SkillProposalEngine).
## Issue Context
Compliance requires a single triggered run to execute the full ordered pipeline: (1) MemoryConsolidator, (2) SessionAnalyzer, (3) HistoricalLogIndex rebuild (if stale), (4) CrossSessionPatternMiner, (5) TrendDetector, (6) feed results to AutoMemoryStore + SkillProposalEngine.
## Fix Focus Areas
- aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java[1774-1828]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java (1)
1607-1618: Consider distinct shutdown priorities for schedulers.Both
LearningMaintenanceSchedulerandDeferredActionScheduler(line 1628) use shutdown priority 87. While they don't appear to share critical resources, using distinct priorities (e.g., 87 and 86) would make shutdown ordering deterministic and easier to reason about.Suggested fix
shutdownManager.register(new ShutdownManager.ShutdownParticipant() { `@Override` public String name() { return "Learning Maintenance Scheduler"; } - `@Override` public int priority() { return 87; } + `@Override` public int priority() { return 86; } `@Override` public void onShutdown() { learningMaintenanceScheduler.stop(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java` around lines 1607 - 1618, The two scheduler shutdown participants both return the same priority (87) which makes shutdown ordering ambiguous; update one of them (either the ShutdownManager.ShutdownParticipant registered for learningMaintenanceScheduler or the one for DeferredActionScheduler) to a distinct integer (e.g., keep learningMaintenanceScheduler.priority() == 87 and change DeferredActionScheduler.priority() to 86, or vice versa) so the shutdown order is deterministic; locate the anonymous ShutdownManager.ShutdownParticipant implementations registered around the learningMaintenanceScheduler and DeferredActionScheduler startup code and change the priority() return value accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 1607-1618: The two scheduler shutdown participants both return the
same priority (87) which makes shutdown ordering ambiguous; update one of them
(either the ShutdownManager.ShutdownParticipant registered for
learningMaintenanceScheduler or the one for DeferredActionScheduler) to a
distinct integer (e.g., keep learningMaintenanceScheduler.priority() == 87 and
change DeferredActionScheduler.priority() to 86, or vice versa) so the shutdown
order is deterministic; locate the anonymous ShutdownManager.ShutdownParticipant
implementations registered around the learningMaintenanceScheduler and
DeferredActionScheduler startup code and change the priority() return value
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d448f68-54b0-479c-9d12-69f4bf7666a6
📒 Files selected for processing (4)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningMaintenanceScheduler.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/LearningMaintenanceSchedulerTest.javaaceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningMaintenanceScheduler.java`:
- Around line 59-70: The start() method sets running=true before
creating/assigning scheduler, allowing stop() to see scheduler==null and miss
shutting down a later-created executor; fix by creating and scheduling the
executor into a local variable first (use the same scheduler creation block and
scheduleAtFixedRate with tickMillis), then attempt to set running via
running.compareAndSet(false, true); if the CAS fails (stop raced in),
immediately shutdownNow/terminate the newly-created executor to avoid leaking
threads and return; update references to the instance field scheduler only after
successfully setting running, and mirror the same serialized lifecycle logic in
stop() to read/clear scheduler safely.
- Around line 139-144: Before calling pipeline.run(trigger.id()) capture the
current sessionsSinceLastRun (e.g., startSessions = sessionsSinceLastRun.get()),
then after pipeline.run(...) compute delta = sessionsSinceLastRun.get() -
startSessions and set sessionsSinceLastRun to delta instead of unconditionally
setting it to 0; this preserves any session-close increments that happened while
the virtual thread was running. Update the code around
Thread.ofVirtual(...){...} where pipeline.run(...), lastRunAt and
sessionsSinceLastRun are handled (symbols: Thread.ofVirtual, pipeline.run,
lastRunAt, sessionsSinceLastRun, Trigger.SIZE_THRESHOLD) and use AtomicInteger
atomic methods (get, set/getAndSet) or a CAS loop to ensure thread-safe update
of the preserved delta.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java`:
- Around line 347-352: The fileSize(Path file) method in AutoMemoryStore is
silently swallowing IOExceptions and returning 0L; change it to catch
IOException, log a low-noise message including the file path and the exception
(e.g., LOGGER.debug/trace or LOGGER.warn with low frequency) so permission/disk
errors are visible, then return 0L as before; ensure you reference the existing
logger instance in AutoMemoryStore (or add one if missing) and include the
exception object in the log call for full stack/causal info.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8340c4fa-1c55-4da2-87c3-8970aacfdc72
📒 Files selected for processing (4)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/LearningMaintenanceScheduler.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/LearningMaintenanceSchedulerTest.javaaceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.javaaceclaw-memory/src/test/java/dev/aceclaw/memory/AutoMemoryStoreTest.java
Comment on lines
+347
to
352
| private static long fileSize(Path file) { | ||
| try { | ||
| return Files.exists(file) ? Files.size(file) : 0L; | ||
| } catch (IOException e) { | ||
| return 0L; | ||
| } |
There was a problem hiding this comment.
Avoid silently swallowing file-size read failures
At Line 350, returning 0L on IOException without logging hides permission/disk issues and can silently disable size-threshold maintenance behavior.
💡 Suggested fix (add low-noise logging)
private static long fileSize(Path file) {
try {
return Files.exists(file) ? Files.size(file) : 0L;
} catch (IOException e) {
+ log.warn("Failed to read backing file size for {}: {}", file, e.getMessage());
return 0L;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static long fileSize(Path file) { | |
| try { | |
| return Files.exists(file) ? Files.size(file) : 0L; | |
| } catch (IOException e) { | |
| return 0L; | |
| } | |
| private static long fileSize(Path file) { | |
| try { | |
| return Files.exists(file) ? Files.size(file) : 0L; | |
| } catch (IOException e) { | |
| log.warn("Failed to read backing file size for {}: {}", file, e.getMessage()); | |
| return 0L; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-memory/src/main/java/dev/aceclaw/memory/AutoMemoryStore.java` around
lines 347 - 352, The fileSize(Path file) method in AutoMemoryStore is silently
swallowing IOExceptions and returning 0L; change it to catch IOException, log a
low-noise message including the file path and the exception (e.g.,
LOGGER.debug/trace or LOGGER.warn with low frequency) so permission/disk errors
are visible, then return 0L as before; ensure you reference the existing logger
instance in AutoMemoryStore (or add one if missing) and include the exception
object in the log call for full stack/causal info.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/memory-system-design.md (1)
3-3: Consider updating the document version.This document retains version "2.1 | 2026-02-18" while
docs/self-learning.mdhas been updated to "2.0 | 2026-03-13" (newer date, lower version number). Since both documents have undergone substantial updates to reflect the deferred maintenance architecture, consider synchronizing version numbers for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/memory-system-design.md` at line 3, The document header contains "Version 2.1 | 2026-02-18" which is inconsistent with docs/self-learning.md ("Version 2.0 | 2026-03-13"); update the version header line (the string "Version 2.1 | 2026-02-18") in memory-system-design.md to a synchronized value (for example "Version 2.2 | 2026-03-13") and ensure any internal metadata or front-matter that references the version/date is updated to the same new version/date so both docs reflect a consistent release identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/memory-system-design.md`:
- Line 3: The document header contains "Version 2.1 | 2026-02-18" which is
inconsistent with docs/self-learning.md ("Version 2.0 | 2026-03-13"); update the
version header line (the string "Version 2.1 | 2026-02-18") in
memory-system-design.md to a synchronized value (for example "Version 2.2 |
2026-03-13") and ensure any internal metadata or front-matter that references
the version/date is updated to the same new version/date so both docs reflect a
consistent release identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a802527f-1405-4ec1-9717-d767dea4dd3d
📒 Files selected for processing (4)
PRD.mdREADME.mddocs/memory-system-design.mddocs/self-learning.md
This was referenced Mar 13, 2026
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
1 participant
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.
Summary
Testing
Closes #127
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation