feat(deferred): MVP agent deferred action with CLI notifications#143
Merged
Conversation
|
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. 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughAdds an agent-driven deferred-action subsystem: models, persistent store, tick-based scheduler, event feed, DeferCheck tool, daemon wiring and RPCs, StreamingAgentHandler integration for resume/queue semantics, CLI polling/rendering of defer events, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as StreamingAgentHandler
participant Tool as DeferCheckTool
participant Scheduler as DeferredActionScheduler
participant Store as DeferredActionStore
participant Lock as SessionTurnLock
participant LLM as LLM/AgentLoop
participant EventBus as EventBus
Agent->>Tool: execute(defer_check: delaySeconds, goal, maxRetries)
Tool->>Scheduler: schedule(sessionId, delaySeconds, goal, maxRetries)
Scheduler->>Store: findByIdempotencyKey(...) / put(DeferredAction)
Scheduler->>EventBus: publish(ActionScheduled)
Tool-->>Agent: return ToolResult(actionId, runAt, expiresAt)
Note over Scheduler: Tick loop evaluates pending actions
Scheduler->>Store: allPending()
Scheduler->>Scheduler: isDue / isExpired
Scheduler->>Lock: tryLock(sessionId)
alt Lock acquired (idle)
Scheduler->>LLM: inject defer prompt into session
LLM-->>Scheduler: result/output
Scheduler->>Store: put(withSuccess / withFailure)
Scheduler->>EventBus: publish(ActionCompleted / ActionFailed)
else Lock not acquired (active)
Scheduler->>Scheduler: queue action for session
Scheduler->>EventBus: publish(ActionQueued)
end
Agent->>Scheduler: notifyTurnComplete(sessionId)
Scheduler->>Scheduler: drain queued actions (acquire Lock, execute)
sequenceDiagram
participant CLI as TerminalRepl
participant Daemon as AceClawDaemon (RPC)
participant Feed as DeferredEventFeed
participant Renderer as UI
CLI->>Daemon: bootstrapDeferEventSeq()
CLI->>Daemon: deferred.events.poll(afterSeq=0, limit=10)
Daemon->>Feed: poll(0,10)
Feed-->>Daemon: PollResult(nextSeq, entries)
Daemon-->>CLI: DeferEvent[]
loop status ticker
CLI->>Daemon: deferred.events.poll(deferEventSeq, limit=10)
Daemon->>Feed: poll(deferEventSeq, limit=10)
Feed-->>Daemon: entries
Daemon-->>CLI: events
CLI->>Renderer: renderDeferredEventNote(event)
CLI->>CLI: deferEventSeq = nextSeq
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferCheckTool.java (1)
9-9: Unused import.The
Objectsimport is declared but not used in this file.♻️ Remove unused import
-import java.util.Objects;🤖 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/deferred/DeferCheckTool.java` at line 9, Remove the unused import of java.util.Objects from the top of DeferCheckTool (the import declaration is unused in the class DeferCheckTool), leaving only the necessary imports; ensure no other references to Objects exist in methods like any equals/hashCode usages before removing.aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredEventFeed.java (1)
69-71: Consider null-guarding List in PollResult record.Per coding guidelines, record constructors should null-guard List fields to prevent NPE and ensure immutability.
♻️ Suggested improvement
- public record PollResult(long nextSequence, List<Entry> entries) {} + public record PollResult(long nextSequence, List<Entry> entries) { + public PollResult { + entries = entries != null ? List.copyOf(entries) : List.of(); + } + }As per coding guidelines: "Always null-guard
Listfields in record constructors:signals = signals != null ? List.copyOf(signals) : List.of()"🤖 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/deferred/DeferredEventFeed.java` around lines 69 - 71, The PollResult record's List field 'entries' must be null-guarded and defensively copied to avoid NPEs and ensure immutability; add a canonical constructor for PollResult(long nextSequence, List<Entry> entries) that sets this.entries = entries != null ? List.copyOf(entries) : List.of() (keeping nextSequence as-is) so callers cannot pass a null or mutable list.aceclaw-daemon/src/test/java/dev/aceclaw/daemon/deferred/DeferredActionSchedulerTest.java (2)
257-279: Test relies on scheduler internal timing - potential flakiness.Starting the scheduler and immediately calling
tick()manually could race with the scheduler's internal tick loop. While this test likely passes because the internal tick interval (5s default) is much longer than the test execution time, it's worth noting this coupling.Consider calling
scheduler.stop()immediately afterscheduler.start()before manual tick manipulation, or add a note that the test relies on the tick interval being longer than test execution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/deferred/DeferredActionSchedulerTest.java` around lines 257 - 279, The test tickExpiresOldActions races with the scheduler's internal loop because it calls scheduler.start() then immediately scheduler.tick(); to fix, avoid running the background loop: either remove the call to scheduler.start() entirely or call scheduler.start() followed immediately by scheduler.stop() before creating the expired action and invoking scheduler.tick(); ensure you still create the expired DeferredAction (new DeferredAction(...)) and put it into store, then call scheduler.tick() and assert the state, so the test uses the explicit tick() only and cannot race with the scheduler's internal thread.
327-331: Test assertion may be fragile due to output format.The test checks for
"actionId:"but the actual output fromDeferCheckTool.execute()contains" actionId: "(with leading spaces and trailing space before the value). The currentcontains()check works, but if the format changes (e.g., to JSON), this test would fail unexpectedly.Consider parsing the response more robustly or testing for the key message "scheduled successfully" which is more stable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/deferred/DeferredActionSchedulerTest.java` around lines 327 - 331, The test DeferredActionSchedulerTest currently asserts that result.output() contains the brittle substring "actionId:" from DeferCheckTool.execute(); instead, make the assertion robust by either parsing the output (e.g., convert to JSON or split lines and extract the actionId key) or simply assert the more stable message "scheduled successfully" and/or that result.output() matches a regex that tolerates spacing around "actionId" (refer to the variables result and tool and the method DeferCheckTool.execute()) so formatting changes won't break the test.aceclaw-infra/src/main/java/dev/aceclaw/infra/event/DeferEvent.java (1)
27-47: Consider adding null validation to event record constructors.The event records don't validate that required fields (actionId, sessionId, timestamp) are non-null. While events are typically created by trusted internal code, defensive validation prevents hard-to-debug NPEs.
This is a low-priority suggestion since events are created by the scheduler which already has validated data. Per coding guidelines: "Use
Objects.requireNonNull(param, "param")on parameters used in.equals()or passed to downstream calls".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-infra/src/main/java/dev/aceclaw/infra/event/DeferEvent.java` around lines 27 - 47, The record event types (ActionScheduled, ActionTriggered, ActionCompleted, ActionFailed, ActionExpired, ActionCancelled, ActionQueued) lack null checks for required fields (e.g., actionId, sessionId, timestamp); add defensive validation by implementing compact canonical constructors for each record and call Objects.requireNonNull(...) for parameters used in equals/hash or passed downstream (at least actionId, sessionId, and timestamp), including descriptive parameter names in the requireNonNull calls to satisfy the coding guideline.aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java (1)
965-982: Consider adding actionId validation before calling cancel.The null/blank check for
actionIdis good, but the scheduler'scancel()method is called without verifying that the scheduler itself checked the action exists. The current implementation returnsfalsefor non-existent actions, which is reasonable, but the response doesn't distinguish between "action not found" and "action was not in PENDING state".This is a minor UX consideration for debugging - no code change required unless you want richer error feedback.
🤖 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 965 - 982, Add explicit validation of the target action before calling deferredActionScheduler.cancel in the router.register("deferred.cancel") handler: query the scheduler (e.g., a method like deferredActionScheduler.get(actionId) / actionExists(actionId) / getScheduledAction(actionId)) to detect missing actions or non-PENDING states, then return a richer JSON result (fields such as "cancelled": boolean, "status": "not_found" | "not_pending" | "cancelled", and "actionId") instead of only the boolean from deferredActionScheduler.cancel; update the handler around deferredActionScheduler.cancel(actionId, reason) to branch on the existence/state check and populate the result accordingly so callers can distinguish "action not found" from "was not in PENDING state".aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java (1)
355-453: Consider validating session existence earlier and improving error context.The
executeActionmethod handles session-gone scenarios correctly (lines 357-366), but the retry backoff logic at lines 437-451 recreates the entireDeferredActionrecord. Consider adding awithRunAt()method toDeferredActionfor cleaner backoff updates.♻️ Suggested improvement for retry action creation
Add a helper method to DeferredAction:
public DeferredAction withRunAt(Instant newRunAt) { return new DeferredAction(actionId, sessionId, idempotencyKey, createdAt, newRunAt, expiresAt, goal, maxRetries, attempts, state, lastError, lastOutput); }Then simplify the retry logic:
- var retryAction = new DeferredAction( - failedAction.actionId(), failedAction.sessionId(), - failedAction.idempotencyKey(), failedAction.createdAt(), - Instant.now().plusSeconds(backoffSeconds), - failedAction.expiresAt(), failedAction.goal(), - failedAction.maxRetries(), failedAction.attempts(), - DeferredActionState.PENDING, failedAction.lastError(), - failedAction.lastOutput()); + var retryAction = failedAction.withRunAt(Instant.now().plusSeconds(backoffSeconds));🤖 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/deferred/DeferredActionScheduler.java` around lines 355 - 453, The retry backoff rebuilds a DeferredAction manually in executeAction; add a convenience method on DeferredAction (e.g., withRunAt(Instant newRunAt)) that returns a new instance with only runAt changed, then replace the manual constructor usage in executeAction's retry branch with failedAction.withRunAt(Instant.now().plusSeconds(backoffSeconds)) to simplify and clarify the backoff update while keeping attempts/state/error/output from failedAction.aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java (1)
594-606: Stop polling deferred events after capability is known unsupported.When
deferred.events.pollis unsupported, this path can still retry every second, producing repetitive failing RPCs and log noise. Cache capability state and short-circuit future polls after the first unsupported response.♻️ Suggested hardening
+ /** False once daemon reports deferred.events.poll is unsupported. */ + private volatile boolean deferredEventsSupported = true; private long bootstrapDeferEventSeq(DaemonConnection conn) throws IOException, DaemonClient.DaemonClientException { var params = client.objectMapper().createObjectNode(); params.put("afterSeq", Long.MAX_VALUE); params.put("limit", 1); try { JsonNode result = conn.sendRequest("deferred.events.poll", params); return Math.max(0L, result.path("nextSeq").asLong(0L)); } catch (Exception e) { + deferredEventsSupported = false; log.debug("deferred.events.poll not available (daemon may not support it): {}", e.getMessage()); return 0L; } } private void pollAndRenderDeferredEvents(DaemonConnection conn, LineReader reader) { - if (conn == null) return; + if (conn == null || !deferredEventsSupported) return; try { var params = client.objectMapper().createObjectNode(); params.put("afterSeq", deferEventSeq); params.put("limit", 20); JsonNode result = conn.sendRequest("deferred.events.poll", params); deferEventSeq = Math.max(deferEventSeq, result.path("nextSeq").asLong(deferEventSeq)); JsonNode events = result.path("events"); if (!events.isArray() || events.isEmpty()) { return; } // ... } catch (Exception e) { + if (e.getMessage() != null && e.getMessage().toLowerCase().contains("not supported")) { + deferredEventsSupported = false; + log.debug("Disabling deferred event polling: {}", e.getMessage()); + return; + } log.debug("Failed to poll deferred events: {}", e.getMessage()); } }Also applies to: 608-644
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 594 - 606, The bootstrapDeferEventSeq method currently retries calling the "deferred.events.poll" RPC even when the daemon doesn't support it; add a cached capability flag (e.g., a private Boolean field like supportsDeferredEvents or AtomicBoolean deferredEventsSupported) on the TerminalRepl instance, set it to false when bootstrapDeferEventSeq catches the unsupported exception, and have bootstrapDeferEventSeq immediately return 0L if the flag is false to short-circuit future polls; also update any other polling code paths that call deferred.events.poll (the polling loop referenced around lines 608-644) to check this cached flag before sending requests so you avoid repeated failing RPCs and log noise.
🤖 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-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 646-711: renderDeferredEventNote currently injects raw goal,
error, and reason strings into ANSI-formatted output, which can include control
chars/newlines/ANSI escapes; create or reuse a helper (e.g.,
sanitizeTerminalText) and call it before rendering goal (in the "scheduled"
case), err (in the "failed" case) and reason (in "cancelled" and "queued") to
strip control characters and ANSI escape sequences, replace newlines with a
single space, and truncate to a safe max length (like 60 chars); use
sanitizeCronSummary for full markdown summaries as-is but ensure the short
inline fields are sanitized before concatenation so renderDeferredEventNote
produces safe terminal output.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Around line 184-208: The schedule() method does not validate the goal
parameter before passing it to computeIdempotencyKey(), which will lead to an
NPE when goal is null; add a null-check at the start of schedule() (e.g., use
Objects.requireNonNull(goal, "goal")) so goal is validated before any use
(including computeIdempotencyKey and any equals/hash operations), and adjust any
related comments or tests to reflect the explicit null contract for
schedule(String sessionId, int delaySeconds, String goal, int maxRetries).
- Around line 327-336: The deferred action path has a race: tick() uses
turnLock.tryLock() then releases it and assumes executeAction() will acquire the
same lock, but executeAction() does not, allowing concurrent mutations; fix by
making executeAction(...) acquire and release the same turnLock for the session
around any modifications to session state (history updates at the locations
referenced in executeAction), or wrap the entire body that mutates session state
in a try/finally that locks turnLock before mutation and unlocks in finally;
update DeferredActionScheduler to use the same turnLock used by tick() and
handlePrompt() so deferred-exec-* virtual threads synchronize correctly with
handlePrompt().
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionStore.java`:
- Around line 135-144: The methods bySession, findByIdempotencyKey, and
pendingCountForSession call .equals() on their input parameters without null
checks; add Objects.requireNonNull(sessionId, "sessionId") in bySession,
Objects.requireNonNull(idempotencyKey, "idempotencyKey") in
findByIdempotencyKey, and Objects.requireNonNull(sessionId, "sessionId") in
pendingCountForSession (or equivalent parameter names in those methods) at the
start of each method so null inputs throw a clear NPE before any .equals()
invocation.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredEventFeed.java`:
- Around line 37-46: The append method currently increments sequence outside the
synchronized(lock) causing entries to be added out of order; move the call to
sequence.incrementAndGet() inside the synchronized (lock) block before creating
and adding new Entry(seq, event) so that the seq assignment and
entries.addLast(new Entry(...)) happen atomically; keep sequence as AtomicLong
if you still need concurrent reads from poll(), otherwise you may convert to a
plain long guarded by the same lock, and ensure maxEvents trimming
(entries.removeFirst()) remains inside the same synchronized block.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.java`:
- Around line 213-218: The lock acquired from
sessionTurnLocks.computeIfAbsent(sessionId, _ -> new ReentrantLock()) (stored in
turnLock) can leak if cancelContext.startMonitor() throws because the
try/finally that unlocks it happens later; move the try immediately after
acquiring turnLock so that cancelContext.startMonitor() and the rest of the
protected work run inside the try, and ensure turnLock.unlock() is called in the
finally (and adjust any outer finally blocks that assumed the lock was still
held) so the lock is always released even when startMonitor() throws.
---
Nitpick comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 594-606: The bootstrapDeferEventSeq method currently retries
calling the "deferred.events.poll" RPC even when the daemon doesn't support it;
add a cached capability flag (e.g., a private Boolean field like
supportsDeferredEvents or AtomicBoolean deferredEventsSupported) on the
TerminalRepl instance, set it to false when bootstrapDeferEventSeq catches the
unsupported exception, and have bootstrapDeferEventSeq immediately return 0L if
the flag is false to short-circuit future polls; also update any other polling
code paths that call deferred.events.poll (the polling loop referenced around
lines 608-644) to check this cached flag before sending requests so you avoid
repeated failing RPCs and log noise.
In `@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.java`:
- Around line 965-982: Add explicit validation of the target action before
calling deferredActionScheduler.cancel in the router.register("deferred.cancel")
handler: query the scheduler (e.g., a method like
deferredActionScheduler.get(actionId) / actionExists(actionId) /
getScheduledAction(actionId)) to detect missing actions or non-PENDING states,
then return a richer JSON result (fields such as "cancelled": boolean, "status":
"not_found" | "not_pending" | "cancelled", and "actionId") instead of only the
boolean from deferredActionScheduler.cancel; update the handler around
deferredActionScheduler.cancel(actionId, reason) to branch on the
existence/state check and populate the result accordingly so callers can
distinguish "action not found" from "was not in PENDING state".
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferCheckTool.java`:
- Line 9: Remove the unused import of java.util.Objects from the top of
DeferCheckTool (the import declaration is unused in the class DeferCheckTool),
leaving only the necessary imports; ensure no other references to Objects exist
in methods like any equals/hashCode usages before removing.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Around line 355-453: The retry backoff rebuilds a DeferredAction manually in
executeAction; add a convenience method on DeferredAction (e.g.,
withRunAt(Instant newRunAt)) that returns a new instance with only runAt
changed, then replace the manual constructor usage in executeAction's retry
branch with failedAction.withRunAt(Instant.now().plusSeconds(backoffSeconds)) to
simplify and clarify the backoff update while keeping
attempts/state/error/output from failedAction.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredEventFeed.java`:
- Around line 69-71: The PollResult record's List field 'entries' must be
null-guarded and defensively copied to avoid NPEs and ensure immutability; add a
canonical constructor for PollResult(long nextSequence, List<Entry> entries)
that sets this.entries = entries != null ? List.copyOf(entries) : List.of()
(keeping nextSequence as-is) so callers cannot pass a null or mutable list.
In
`@aceclaw-daemon/src/test/java/dev/aceclaw/daemon/deferred/DeferredActionSchedulerTest.java`:
- Around line 257-279: The test tickExpiresOldActions races with the scheduler's
internal loop because it calls scheduler.start() then immediately
scheduler.tick(); to fix, avoid running the background loop: either remove the
call to scheduler.start() entirely or call scheduler.start() followed
immediately by scheduler.stop() before creating the expired action and invoking
scheduler.tick(); ensure you still create the expired DeferredAction (new
DeferredAction(...)) and put it into store, then call scheduler.tick() and
assert the state, so the test uses the explicit tick() only and cannot race with
the scheduler's internal thread.
- Around line 327-331: The test DeferredActionSchedulerTest currently asserts
that result.output() contains the brittle substring "actionId:" from
DeferCheckTool.execute(); instead, make the assertion robust by either parsing
the output (e.g., convert to JSON or split lines and extract the actionId key)
or simply assert the more stable message "scheduled successfully" and/or that
result.output() matches a regex that tolerates spacing around "actionId" (refer
to the variables result and tool and the method DeferCheckTool.execute()) so
formatting changes won't break the test.
In `@aceclaw-infra/src/main/java/dev/aceclaw/infra/event/DeferEvent.java`:
- Around line 27-47: The record event types (ActionScheduled, ActionTriggered,
ActionCompleted, ActionFailed, ActionExpired, ActionCancelled, ActionQueued)
lack null checks for required fields (e.g., actionId, sessionId, timestamp); add
defensive validation by implementing compact canonical constructors for each
record and call Objects.requireNonNull(...) for parameters used in equals/hash
or passed downstream (at least actionId, sessionId, and timestamp), including
descriptive parameter names in the requireNonNull calls to satisfy the coding
guideline.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawConfig.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/AceClawDaemon.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferCheckTool.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredAction.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionState.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionStore.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredEventFeed.javaaceclaw-daemon/src/test/java/dev/aceclaw/daemon/deferred/DeferredActionSchedulerTest.javaaceclaw-infra/src/main/java/dev/aceclaw/infra/event/AceClawEvent.javaaceclaw-infra/src/main/java/dev/aceclaw/infra/event/DeferEvent.java
Greptile SummaryThis PR implements MVP agent deferred action - allowing the agent to schedule delayed checks via the Key Changes:
Critical Issues:
Architecture: Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant DeferCheckTool
participant Scheduler
participant Store
participant EventBus
participant CLI
participant Session
participant Handler
Agent->>DeferCheckTool: defer_check(60s, "check build")
DeferCheckTool->>Scheduler: schedule(sessionId, 60, goal)
Scheduler->>Store: findByIdempotencyKey()
Store-->>Scheduler: empty (new action)
Scheduler->>Store: put(action)
Scheduler->>Store: save()
Scheduler->>EventBus: publish(ActionScheduled)
EventBus->>CLI: event(scheduled)
CLI->>CLI: render "[deferred scheduled]"
Note over Scheduler: 60 seconds pass...
Scheduler->>Scheduler: tick()
Scheduler->>Store: allPending()
Store-->>Scheduler: [action]
Scheduler->>Handler: tryLock(sessionId)
Handler-->>Scheduler: success (idle)
Scheduler->>Scheduler: unlock & spawn thread
Note right of Scheduler: BUG: executeAction<br/>never re-acquires lock!
Scheduler->>Session: addMessage(system)
Scheduler->>Session: messages()
Note over Scheduler,Session: Race: user turn could<br/>run here concurrently
Scheduler->>Scheduler: agentLoop.runTurn()
Scheduler->>Session: addMessage(result)
Scheduler->>Store: put(action.withSuccess())
Scheduler->>EventBus: publish(ActionCompleted)
EventBus->>CLI: event(completed)
CLI->>CLI: render "--- deferred completed ---"
Last reviewed commit: fe67a5a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+327
to
+336
| // Try to acquire the session turn lock | ||
| var turnLock = sessionTurnLocks.computeIfAbsent( | ||
| action.sessionId(), _ -> new ReentrantLock()); | ||
|
|
||
| if (turnLock.tryLock()) { | ||
| // Session is idle — execute on a virtual thread while holding the lock. | ||
| // The virtual thread inherits the lock context; we pass it explicitly | ||
| // so executeAction can release it when done. | ||
| final var actionToRun = action; | ||
| final var heldLock = turnLock; |
There was a problem hiding this comment.
race condition: lock released before executeAction runs
the turn lock is acquired, immediately released, then executeAction spawns on a virtual thread. but executeAction never re-acquires the lock before accessing session.messages() (lines 381, 401, 410). between releasing the lock and the virtual thread starting, a new user turn could acquire the lock and run concurrently with the deferred action, causing:
- race conditions on
session.addMessage()calls - interleaved conversation history
- concurrent LLM requests for same session
fix: executeAction should acquire sessionTurnLocks.get(action.sessionId()) before line 381, and release it at the end. same issue exists in notifyTurnComplete (line 275) which also spawns threads without acquiring the lock
Suggested change
| // Try to acquire the session turn lock | |
| var turnLock = sessionTurnLocks.computeIfAbsent( | |
| action.sessionId(), _ -> new ReentrantLock()); | |
| if (turnLock.tryLock()) { | |
| // Session is idle — execute on a virtual thread while holding the lock. | |
| // The virtual thread inherits the lock context; we pass it explicitly | |
| // so executeAction can release it when done. | |
| final var actionToRun = action; | |
| final var heldLock = turnLock; | |
| if (turnLock.tryLock()) { | |
| try { | |
| // Session is idle — mark it and execute on virtual thread | |
| // The virtual thread will acquire the lock in executeAction() | |
| } finally { | |
| turnLock.unlock(); | |
| } | |
| final var actionToRun = action; | |
| Thread.ofVirtual().name("deferred-exec-" + action.actionId()).start( | |
| () -> executeAction(actionToRun)); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java
Line: 327-336
Comment:
race condition: lock released before `executeAction` runs
the turn lock is acquired, immediately released, then `executeAction` spawns on a virtual thread. but `executeAction` never re-acquires the lock before accessing `session.messages()` (lines 381, 401, 410). between releasing the lock and the virtual thread starting, a new user turn could acquire the lock and run concurrently with the deferred action, causing:
- race conditions on `session.addMessage()` calls
- interleaved conversation history
- concurrent LLM requests for same session
fix: `executeAction` should acquire `sessionTurnLocks.get(action.sessionId())` before line 381, and release it at the end. same issue exists in `notifyTurnComplete` (line 275) which also spawns threads without acquiring the lock
```suggestion
if (turnLock.tryLock()) {
try {
// Session is idle — mark it and execute on virtual thread
// The virtual thread will acquire the lock in executeAction()
} finally {
turnLock.unlock();
}
final var actionToRun = action;
Thread.ofVirtual().name("deferred-exec-" + action.actionId()).start(
() -> executeAction(actionToRun));
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+85
to
+89
| /** Per-session turn locks — shared with StreamingAgentHandler. */ | ||
| private final ConcurrentHashMap<String, ReentrantLock> sessionTurnLocks; | ||
|
|
||
| /** Actions queued because session was busy during tick. */ | ||
| private final ConcurrentHashMap<String, Queue<DeferredAction>> queuedActions = |
There was a problem hiding this comment.
memory leak: these maps never remove old session entries
sessionTurnLocks and queuedActions grow unbounded as sessions are created. consider removing entries when sessions are destroyed or become inactive
Prompt To Fix With AI
This is a comment left during a code review.
Path: aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java
Line: 85-89
Comment:
memory leak: these maps never remove old session entries
`sessionTurnLocks` and `queuedActions` grow unbounded as sessions are created. consider removing entries when sessions are destroyed or become inactive
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java (1)
268-281:⚠️ Potential issue | 🔴 CriticalQueued deferred actions execute without taking the session turn lock.
Line 279 starts
executeActiondirectly fromnotifyTurnComplete, butexecuteActiondoes not acquire the per-session lock. This can run concurrently with a new prompt turn and interleave session-history mutations (Line 392, Line 421).Run this read-only check to confirm the lockless queue-drain path and lock usage sites:
#!/bin/bash set -euo pipefail file="aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java" echo "== notifyTurnComplete drain path ==" rg -n -C3 'notifyTurnComplete|executeAction\(actionToRun\)|queue\.poll' "$file" echo echo "== executeAction lock acquisition sites ==" rg -n -C3 'private void executeAction|sessionTurnLocks\.computeIfAbsent|turnLock\.lock\(' "$file"Expected result:
notifyTurnCompletecallsexecuteAction(actionToRun)whileexecuteActionitself has no lock acquisition.Also applies to: 366-382, 419-422
🤖 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/deferred/DeferredActionScheduler.java` around lines 268 - 281, notifyTurnComplete drains queued actions onto virtual threads and calls executeAction(actionToRun) without acquiring the per-session turn lock (sessionTurnLocks / turnLock), allowing concurrent session-history mutations; fix by ensuring the per-session lock is acquired around the action execution: when spawning the virtual thread in notifyTurnComplete (and other drain sites at the same pattern), wrap the Runnable so it obtains the lock from sessionTurnLocks.computeIfAbsent(sessionId, ...) and locks/unlocks around calling executeAction(actionToRun) (or call a new helper like runWithTurnLock(sessionId, () -> executeAction(...))) so executeAction always runs while holding the session turn lock.
🧹 Nitpick comments (2)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java (1)
608-644: LGTM with optional suggestion.The implementation correctly mirrors the scheduler events polling pattern. The duplication between
pollAndRenderSchedulerEventsandpollAndRenderDeferredEventscould be consolidated into a generic polling helper, but this is a low-priority refactor given the clear readability of the current approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java` around lines 608 - 644, There is duplication between pollAndRenderSchedulerEvents and pollAndRenderDeferredEvents; extract the shared polling/rendering logic into a reusable helper (e.g., pollAndRenderEvents) that accepts the DaemonConnection, LineReader, the request method name ("scheduler.events.poll" or "deferred.events.poll"), and a small strategy/lambda for per-event handling (e.g., render note, enqueueUiPrintAbove vs enqueueUiNotice, and any short-note construction) so both pollAndRenderSchedulerEvents and pollAndRenderDeferredEvents delegate to that helper while keeping their event-specific behavior in the passed handler.aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionStore.java (1)
42-45: Add explicit null contracts for constructor andput()inputs.Line 43 and Line 181 dereference inputs without clear fail-fast contracts. Explicit
Objects.requireNonNull(...)makes failure mode deterministic and clearer.🔧 Proposed fix (fail-fast null checks)
+import java.util.Objects; ... public DeferredActionStore(Path homeDir) { + Objects.requireNonNull(homeDir, "homeDir"); this.deferredDir = homeDir.resolve("deferred"); this.actionsFile = deferredDir.resolve(ACTIONS_FILE); ... public void put(DeferredAction action) { + Objects.requireNonNull(action, "action"); + Objects.requireNonNull(action.actionId(), "action.actionId"); lock.writeLock().lock(); try { actions.put(action.actionId(), action);Based on learnings: Use
Objects.requireNonNull(param, "param")on parameters used in.equals()or passed to downstream calls.Also applies to: 178-182
🤖 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/deferred/DeferredActionStore.java` around lines 42 - 45, Add explicit fail-fast null checks using Objects.requireNonNull for the DeferredActionStore constructor parameter and for the put(...) method parameter: in the DeferredActionStore constructor (which sets deferredDir and actionsFile) call Objects.requireNonNull(homeDir, "homeDir") before resolving paths, and in the put(...) method call Objects.requireNonNull(action, "action") (and any other method parameters used without checks) before dereferencing or calling equals()/passing downstream; this ensures deterministic NPEs and clearer contracts for DeferredActionStore and its put() operation.
🤖 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/deferred/DeferCheckTool.java`:
- Around line 122-123: The code in DeferCheckTool is using
JsonNode.asInt()/asText() on the input JsonNode which silently coerces wrong
types (e.g., null -> "null") for fields like delaySeconds and goal; update the
parsing to perform strict type checks on input (use input.has(...) and
JsonNode.isInt()/isNumber()/isTextual()/isNull() as appropriate) before
converting, and throw or return a clear validation error if a field is missing
or of the wrong type; apply the same strict checks to the other fields
referenced around lines 125-127 and 129 (same input JsonNode access) so all
inputs are validated consistently.
- Around line 22-26: DeferCheckTool currently stores a shared mutable session
context in the volatile field currentSessionId which can be overwritten by
concurrent requests; remove reliance on this field and instead thread-confine
session binding by passing the sessionId as an explicit parameter to the methods
that schedule or operate on DeferredActionScheduler (e.g., replace uses of
currentSessionId in schedule/execute methods with a new sessionId parameter), or
make DeferCheckTool instance-per-request so scheduler and sessionId are not
shared; update all call sites that reference currentSessionId (and any methods
using scheduler with that field) to provide the sessionId argument and eliminate
the mutable field to prevent cross-session interference.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Around line 349-355: The queuedActions logic in DeferredActionScheduler is
enqueuing the same action repeatedly when sessions remain busy; modify the
queuing to deduplicate by session by tracking queued action IDs (e.g., maintain
a ConcurrentHashMap<SessionId, Set<ActionId>> alongside queuedActions or replace
the queue with a structure that prevents duplicates) and check that set before
offering (and add to it when you offer, remove when draining). Update all places
that enqueue (the block shown and the earlier 268-281 enqueue code paths) to use
the same dedupe mechanism and ensure removal from the per-session set when an
action is executed or cancelled so future re-queues are allowed.
- Around line 197-204: In DeferredActionScheduler, run the idempotency dedupe
lookup that checks for an existing pending action before enforcing the
per-session and global limits (the checks using store.pendingCountForSession /
MAX_PER_SESSION and store.totalPendingCount / MAX_GLOBAL); reorder the logic so
the dedupe returns the existing action immediately for duplicates, and only when
no duplicate is found proceed to evaluate the per-session/global limits and
create the new pending action — apply this same reorder to the other similar
block (the 206-212 occurrence) as well.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionStore.java`:
- Around line 60-64: In DeferredActionStore.load(), guard against null or
invalid deserialization from mapper.readValue(actionsFile.toFile(), new
TypeReference<List<DeferredAction>>() {}) by checking the returned List before
iterating: if the List is null, empty, or contains null elements, handle
gracefully (e.g., log a warning and skip nulls or throw a clear
IllegalStateException). When iterating the loaded list, verify each
DeferredAction is non-null and that action.actionId() is non-null/valid before
calling actions.put(...). Use the existing symbols mapper.readValue,
actionsFile, DeferredAction, action.actionId(), and actions to implement these
defensive checks and fail fast with a meaningful error if the root result is
invalid.
---
Duplicate comments:
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Around line 268-281: notifyTurnComplete drains queued actions onto virtual
threads and calls executeAction(actionToRun) without acquiring the per-session
turn lock (sessionTurnLocks / turnLock), allowing concurrent session-history
mutations; fix by ensuring the per-session lock is acquired around the action
execution: when spawning the virtual thread in notifyTurnComplete (and other
drain sites at the same pattern), wrap the Runnable so it obtains the lock from
sessionTurnLocks.computeIfAbsent(sessionId, ...) and locks/unlocks around
calling executeAction(actionToRun) (or call a new helper like
runWithTurnLock(sessionId, () -> executeAction(...))) so executeAction always
runs while holding the session turn lock.
---
Nitpick comments:
In `@aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.java`:
- Around line 608-644: There is duplication between pollAndRenderSchedulerEvents
and pollAndRenderDeferredEvents; extract the shared polling/rendering logic into
a reusable helper (e.g., pollAndRenderEvents) that accepts the DaemonConnection,
LineReader, the request method name ("scheduler.events.poll" or
"deferred.events.poll"), and a small strategy/lambda for per-event handling
(e.g., render note, enqueueUiPrintAbove vs enqueueUiNotice, and any short-note
construction) so both pollAndRenderSchedulerEvents and
pollAndRenderDeferredEvents delegate to that helper while keeping their
event-specific behavior in the passed handler.
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionStore.java`:
- Around line 42-45: Add explicit fail-fast null checks using
Objects.requireNonNull for the DeferredActionStore constructor parameter and for
the put(...) method parameter: in the DeferredActionStore constructor (which
sets deferredDir and actionsFile) call Objects.requireNonNull(homeDir,
"homeDir") before resolving paths, and in the put(...) method call
Objects.requireNonNull(action, "action") (and any other method parameters used
without checks) before dereferencing or calling equals()/passing downstream;
this ensures deterministic NPEs and clearer contracts for DeferredActionStore
and its put() operation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
aceclaw-cli/src/main/java/dev/aceclaw/cli/TerminalRepl.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/StreamingAgentHandler.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferCheckTool.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionStore.javaaceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredEventFeed.java
🚧 Files skipped from review as they are similar to previous changes (1)
- aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredEventFeed.java
Comment on lines
+22
to
+26
| private volatile DeferredActionScheduler scheduler; | ||
|
|
||
| /** Session ID is injected per-request by the handler. */ | ||
| private volatile String currentSessionId; | ||
|
|
There was a problem hiding this comment.
Avoid shared mutable session context in DeferCheckTool.
Line 25 and Line 135 rely on a mutable currentSessionId field on a shared tool instance. Concurrent prompts from different sessions can overwrite this value and schedule deferred actions under the wrong session.
🔧 Proposed fix (thread-confined session binding)
- /** Session ID is injected per-request by the handler. */
- private volatile String currentSessionId;
+ /** Session ID is bound to the current request thread. */
+ private final ThreadLocal<String> currentSessionId = new ThreadLocal<>();
...
public void setCurrentSessionId(String sessionId) {
- this.currentSessionId = sessionId;
+ if (sessionId == null) {
+ currentSessionId.remove();
+ } else {
+ currentSessionId.set(sessionId);
+ }
}
...
- String sessionId = currentSessionId;
+ String sessionId = currentSessionId.get();
if (sessionId == null || sessionId.isBlank()) {
return new ToolResult("No active session (internal error)", true);
}Also applies to: 41-43, 135-138
🤖 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/deferred/DeferCheckTool.java`
around lines 22 - 26, DeferCheckTool currently stores a shared mutable session
context in the volatile field currentSessionId which can be overwritten by
concurrent requests; remove reliance on this field and instead thread-confine
session binding by passing the sessionId as an explicit parameter to the methods
that schedule or operate on DeferredActionScheduler (e.g., replace uses of
currentSessionId in schedule/execute methods with a new sessionId parameter), or
make DeferCheckTool instance-per-request so scheduler and sessionId are not
shared; update all call sites that reference currentSessionId (and any methods
using scheduler with that field) to provide the sessionId argument and eliminate
the mutable field to prevent cross-session interference.
Comment on lines
+122
to
+123
| int delaySeconds = input.get("delaySeconds").asInt(); | ||
| String goal = input.get("goal").asText(); |
There was a problem hiding this comment.
Validate JSON field types before coercion.
Line 122 (asInt) and Line 123 (asText) silently coerce invalid types, which can produce misleading errors (or allow "goal": null to pass as "null").
🔧 Proposed fix (strict type checks)
- int delaySeconds = input.get("delaySeconds").asInt();
- String goal = input.get("goal").asText();
+ var delayNode = input.get("delaySeconds");
+ if (delayNode == null || !delayNode.isInt()) {
+ return new ToolResult("delaySeconds must be an integer", true);
+ }
+ int delaySeconds = delayNode.intValue();
+
+ var goalNode = input.get("goal");
+ if (goalNode == null || !goalNode.isTextual()) {
+ return new ToolResult("goal must be a string", true);
+ }
+ String goal = goalNode.textValue();Also applies to: 125-127, 129-129
🤖 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/deferred/DeferCheckTool.java`
around lines 122 - 123, The code in DeferCheckTool is using
JsonNode.asInt()/asText() on the input JsonNode which silently coerces wrong
types (e.g., null -> "null") for fields like delaySeconds and goal; update the
parsing to perform strict type checks on input (use input.has(...) and
JsonNode.isInt()/isNumber()/isTextual()/isNull() as appropriate) before
converting, and throw or return a clear validation error if a field is missing
or of the wrong type; apply the same strict checks to the other fields
referenced around lines 125-127 and 129 (same input JsonNode access) so all
inputs are validated consistently.
Comment on lines
+60
to
+64
| List<DeferredAction> loaded = mapper.readValue( | ||
| actionsFile.toFile(), new TypeReference<List<DeferredAction>>() {}); | ||
| for (DeferredAction action : loaded) { | ||
| actions.put(action.actionId(), action); | ||
| } |
There was a problem hiding this comment.
Harden load() against null/invalid deserialization results.
Line 62 and Line 63 assume every deserialized entry is non-null with a valid actionId. A null root or null entries can raise NPE and break scheduler startup unexpectedly.
🔧 Proposed fix (defensive load path)
- List<DeferredAction> loaded = mapper.readValue(
- actionsFile.toFile(), new TypeReference<List<DeferredAction>>() {});
- for (DeferredAction action : loaded) {
- actions.put(action.actionId(), action);
- }
+ List<DeferredAction> loaded = mapper.readValue(
+ actionsFile.toFile(), new TypeReference<List<DeferredAction>>() {});
+ if (loaded == null) {
+ log.warn("Deferred action file {} contained null payload; starting empty", actionsFile);
+ return;
+ }
+ for (DeferredAction action : loaded) {
+ if (action == null || action.actionId() == null || action.actionId().isBlank()) {
+ log.warn("Skipping invalid deferred action entry in {}", actionsFile);
+ continue;
+ }
+ actions.put(action.actionId(), action);
+ }As per coding guidelines: "Check for null return values before using methods that may return null (e.g., Jackson readTree(\"\")) and throw meaningful exceptions."
🤖 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/deferred/DeferredActionStore.java`
around lines 60 - 64, In DeferredActionStore.load(), guard against null or
invalid deserialization from mapper.readValue(actionsFile.toFile(), new
TypeReference<List<DeferredAction>>() {}) by checking the returned List before
iterating: if the List is null, empty, or contains null elements, handle
gracefully (e.g., log a warning and skip nulls or throw a clear
IllegalStateException). When iterating the loaded list, verify each
DeferredAction is non-null and that action.actionId() is non-null/valid before
calling actions.put(...). Use the existing symbols mapper.readValue,
actionsFile, DeferredAction, action.actionId(), and actions to implement these
defensive checks and fail fast with a meaningful error if the root result is
invalid.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java (2)
198-213:⚠️ Potential issue | 🟠 MajorRun idempotency dedupe before limit checks.
At Line 198 and Line 202, duplicates can fail with “limit reached” instead of returning the existing pending action. Move idempotency lookup ahead of per-session/global limit checks.
🔧 Proposed reorder
- if (store.pendingCountForSession(sessionId) >= MAX_PER_SESSION) { - throw new IllegalArgumentException( - "Per-session limit reached (" + MAX_PER_SESSION + " pending actions)"); - } - if (store.totalPendingCount() >= MAX_GLOBAL) { - throw new IllegalArgumentException( - "Global limit reached (" + MAX_GLOBAL + " pending actions)"); - } - // Idempotency check String idempotencyKey = computeIdempotencyKey(sessionId, goal); var existing = store.findByIdempotencyKey(idempotencyKey); if (existing.isPresent()) { log.info("Dedup: returning existing action {} for idempotency key", existing.get().actionId()); return existing.get(); } + + if (store.pendingCountForSession(sessionId) >= MAX_PER_SESSION) { + throw new IllegalArgumentException( + "Per-session limit reached (" + MAX_PER_SESSION + " pending actions)"); + } + if (store.totalPendingCount() >= MAX_GLOBAL) { + throw new IllegalArgumentException( + "Global limit reached (" + MAX_GLOBAL + " pending actions)"); + }🤖 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/deferred/DeferredActionScheduler.java` around lines 198 - 213, The idempotency lookup must run before enforcing limits: in DeferredActionScheduler, call computeIdempotencyKey(sessionId, goal) and check store.findByIdempotencyKey(idempotencyKey) first (returning the existing action when present) before invoking store.pendingCountForSession(sessionId) and store.totalPendingCount() and throwing the per-session/global IllegalArgumentException; move the existing idempotency block (including the log and return of existing.get()) above the limit checks so duplicates are deduped instead of failing with "limit reached".
270-291:⚠️ Potential issue | 🟠 MajorDeduplicate queued actions per session/actionId.
At Line 362, the same due action is queued every tick while the session is busy. During drain (Line 275+), duplicates can spawn repeated executions/events for one action.
🧩 Proposed dedupe approach
+ /** Dedup set for queued action IDs per session. */ + private final ConcurrentHashMap<String, java.util.Set<String>> queuedActionIds = + new ConcurrentHashMap<>(); ... - queuedActions.computeIfAbsent(action.sessionId(), _ -> new ConcurrentLinkedQueue<>()) - .offer(action); - publishEvent(new DeferEvent.ActionQueued( - action.actionId(), action.sessionId(), - "Session busy", Instant.now())); + var ids = queuedActionIds.computeIfAbsent(action.sessionId(), _ -> ConcurrentHashMap.newKeySet()); + if (ids.add(action.actionId())) { + queuedActions.computeIfAbsent(action.sessionId(), _ -> new ConcurrentLinkedQueue<>()) + .offer(action); + publishEvent(new DeferEvent.ActionQueued( + action.actionId(), action.sessionId(), + "Session busy", Instant.now())); + } ... DeferredAction queued; while ((queued = queue.poll()) != null) { + var ids = queuedActionIds.get(sessionId); + if (ids != null) { + ids.remove(queued.actionId()); + } // Re-check that the action is still pending (may have been cancelled)Also applies to: 362-367
🤖 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/deferred/DeferredActionScheduler.java` around lines 270 - 291, The drain loop in DeferredActionScheduler can run the same action multiple times because duplicates are enqueued each tick; fix by deduplicating by actionId per session: when draining queuedActions.get(sessionId) (the variable queue used with queue.poll()), maintain a local HashSet of seen actionIds and skip any queued action whose actionId is already in the set before checking store and scheduling execution (so executeAction is only invoked once per actionId); additionally, prevent duplicate enqueueing at the tick-side by changing the enqueue logic (where the queue is populated at the tick, lines referenced in the review) to check for an existing actionId in the session queue before adding (or switch the per-session queue to a set-like structure) so queuedActions and the drain loop both operate on unique actionIds and sessionTurnLocks usage remains unchanged.
🧹 Nitpick comments (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java (1)
528-528: Guard hash-prefix substring length explicitly.Line 528 slices
substring(0, 16)without a length check. Add a guard even if SHA-256 currently guarantees size, to keep guideline compliance and future-proofing.🛠️ Proposed guard
- String hash = HexFormat.of().formatHex(digest.digest()).substring(0, 16); + String fullHash = HexFormat.of().formatHex(digest.digest()); + String hash = fullHash.length() > 16 ? fullHash.substring(0, 16) : fullHash;As per coding guidelines, "Always check string length before calling
substring():s.length() > max ? s.substring(0, max) + \"...\" : s."🤖 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/deferred/DeferredActionScheduler.java` at line 528, In DeferredActionScheduler, the creation of the hash uses HexFormat.of().formatHex(digest.digest()).substring(0, 16) without checking length; change it to compute the full hex string first (e.g., assign to a variable like fullHash), then guard before taking a prefix: if fullHash.length() > 16 use fullHash.substring(0, 16) (optionally append "..." per guideline) else use fullHash; update the variable name (hash) accordingly and keep uses of digest and HexFormat.of() unchanged.
🤖 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/deferred/DeferredActionScheduler.java`:
- Around line 150-167: The stop() method currently only shuts down the scheduler
and clears queuedActions while virtual worker threads (spawned elsewhere in
DeferredActionScheduler) can continue mutating state; update
DeferredActionScheduler to track those workers (e.g. store
CompletableFuture/Thread/TaskHandle instances when you create virtual threads at
the spawn sites referenced near lines ~280 and ~349) and in stop() first signal
cancellation, await their termination with a bounded timeout, cancel/interrupt
any still-running tasks, then proceed to shutdown the scheduler and clear
queuedActions; ensure this uses a threadsafe collection for worker handles and
preserves Thread.currentThread().interrupt() behavior on InterruptedException.
- Around line 343-360: The tryLock block in DeferredActionScheduler can skip
releasing turnLock if Thread.ofVirtual().start(...) throws; wrap the
virtual-thread creation/start and the tick-thread's turnLock.unlock() in a
try/finally so the original lock is always released even on thread-creation
failure. Specifically, around the code that creates/starts the virtual thread
(the Thread.ofVirtual().name("deferred-exec-"+ action.actionId()).start(...)
call) ensure you perform start inside a try and call turnLock.unlock() in the
finally, while keeping the virtual thread body acquiring heldLock and calling
executeAction(actionToRun) and releasing heldLock in its own finally. This
guarantees the session lock held by the tick thread is released if thread
creation fails.
---
Duplicate comments:
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Around line 198-213: The idempotency lookup must run before enforcing limits:
in DeferredActionScheduler, call computeIdempotencyKey(sessionId, goal) and
check store.findByIdempotencyKey(idempotencyKey) first (returning the existing
action when present) before invoking store.pendingCountForSession(sessionId) and
store.totalPendingCount() and throwing the per-session/global
IllegalArgumentException; move the existing idempotency block (including the log
and return of existing.get()) above the limit checks so duplicates are deduped
instead of failing with "limit reached".
- Around line 270-291: The drain loop in DeferredActionScheduler can run the
same action multiple times because duplicates are enqueued each tick; fix by
deduplicating by actionId per session: when draining
queuedActions.get(sessionId) (the variable queue used with queue.poll()),
maintain a local HashSet of seen actionIds and skip any queued action whose
actionId is already in the set before checking store and scheduling execution
(so executeAction is only invoked once per actionId); additionally, prevent
duplicate enqueueing at the tick-side by changing the enqueue logic (where the
queue is populated at the tick, lines referenced in the review) to check for an
existing actionId in the session queue before adding (or switch the per-session
queue to a set-like structure) so queuedActions and the drain loop both operate
on unique actionIds and sessionTurnLocks usage remains unchanged.
---
Nitpick comments:
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Line 528: In DeferredActionScheduler, the creation of the hash uses
HexFormat.of().formatHex(digest.digest()).substring(0, 16) without checking
length; change it to compute the full hex string first (e.g., assign to a
variable like fullHash), then guard before taking a prefix: if fullHash.length()
> 16 use fullHash.substring(0, 16) (optionally append "..." per guideline) else
use fullHash; update the variable name (hash) accordingly and keep uses of
digest and HexFormat.of() unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java (2)
212-227:⚠️ Potential issue | 🟠 MajorRun idempotency dedupe before pending-limit checks.
The idempotency check at lines 221-227 runs after the limit checks at lines 212-219. A duplicate request for an already-pending action will fail with "limit reached" instead of returning the existing action.
🔧 Proposed fix (reorder checks)
+ // Idempotency check — must run before limit checks + String idempotencyKey = computeIdempotencyKey(sessionId, goal); + var existing = store.findByIdempotencyKey(idempotencyKey); + if (existing.isPresent()) { + log.info("Dedup: returning existing action {} for idempotency key", existing.get().actionId()); + return existing.get(); + } + if (store.pendingCountForSession(sessionId) >= MAX_PER_SESSION) { throw new IllegalArgumentException( "Per-session limit reached (" + MAX_PER_SESSION + " pending actions)"); } if (store.totalPendingCount() >= MAX_GLOBAL) { throw new IllegalArgumentException( "Global limit reached (" + MAX_GLOBAL + " pending actions)"); } - - // Idempotency check - String idempotencyKey = computeIdempotencyKey(sessionId, goal); - var existing = store.findByIdempotencyKey(idempotencyKey); - if (existing.isPresent()) { - log.info("Dedup: returning existing action {} for idempotency key", existing.get().actionId()); - return existing.get(); - }🤖 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/deferred/DeferredActionScheduler.java` around lines 212 - 227, The idempotency dedupe (computeIdempotencyKey + store.findByIdempotencyKey) must run before the pending-limit checks to avoid rejecting duplicate requests; in DeferredActionScheduler move the idempotency block (computeIdempotencyKey, var existing = store.findByIdempotencyKey(...), logging and return of existing.get()) so it executes prior to calling store.pendingCountForSession(...) and store.totalPendingCount(...), leaving the limit checks intact for new actions only.
380-388:⚠️ Potential issue | 🟠 MajorBusy-session ticks can enqueue the same action repeatedly.
When a session is busy across multiple tick intervals, the same
PENDINGaction is offered toqueuedActionson every tick (line 382-383) without deduplication. This can cause duplicateActionQueuedevents and potential double-execution when drained.🔧 Proposed fix (dedupe queued action IDs)
+ /** Dedupe set for queued action IDs per session. */ + private final ConcurrentHashMap<String, java.util.Set<String>> queuedActionIds = + new ConcurrentHashMap<>(); ... } else { // Session is busy — queue for drain on notifyTurnComplete - queuedActions.computeIfAbsent(action.sessionId(), _ -> new ConcurrentLinkedQueue<>()) - .offer(action); - publishEvent(new DeferEvent.ActionQueued( - action.actionId(), action.sessionId(), - "Session busy", Instant.now())); - log.debug("Deferred action '{}' queued (session busy)", action.actionId()); + var ids = queuedActionIds.computeIfAbsent(action.sessionId(), _ -> ConcurrentHashMap.newKeySet()); + if (ids.add(action.actionId())) { + queuedActions.computeIfAbsent(action.sessionId(), _ -> new ConcurrentLinkedQueue<>()) + .offer(action); + publishEvent(new DeferEvent.ActionQueued( + action.actionId(), action.sessionId(), + "Session busy", Instant.now())); + log.debug("Deferred action '{}' queued (session busy)", action.actionId()); + } }Also remove from
queuedActionIdswhen draining innotifyTurnComplete().🤖 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/deferred/DeferredActionScheduler.java` around lines 380 - 388, The busy-session branch currently offers the same PENDING action into queuedActions every tick, causing duplicate ActionQueued events and duplicate entries; modify the enqueue logic in DeferredActionScheduler so it deduplicates by action ID (use a concurrent Set like queuedActionIds alongside queuedActions) — before calling queuedActions.computeIfAbsent(...).offer(action) check-and-add the actionId into queuedActionIds (only offer if add returns true) and still publish ActionQueued once; additionally, when draining in notifyTurnComplete() remove the actionId from queuedActionIds as you poll/dispatch to prevent stale IDs from blocking future re-queues.
🧹 Nitpick comments (2)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java (2)
261-263: Add null validation foractionIdparameter.The
actionIdparameter is passed directly tostore.get()without validation.🛡️ Proposed fix
public boolean cancel(String actionId, String reason) { + Objects.requireNonNull(actionId, "actionId"); var opt = store.get(actionId);🤖 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/deferred/DeferredActionScheduler.java` around lines 261 - 263, The cancel method in DeferredActionScheduler currently calls store.get(actionId) without validating actionId; add a null (and optionally blank) check at the start of DeferredActionScheduler.cancel to immediately return false (or throw IllegalArgumentException if project convention prefers) when actionId is null/empty, so store.get(...) is never invoked with a null key; update the method signature body around the existing store.get(...) call to perform this guard before calling store.get(actionId).
419-420: Clarify attempt logging format.The log message
"(attempt {}/{})"usesattempts()andmaxRetries(), which is misleading. It suggests "attempt X of Y total" butmaxRetriesis the retry limit, not total attempts.📝 Suggested improvement
- log.info("Executing deferred action '{}' (attempt {}/{})", - action.actionId(), runningAction.attempts(), runningAction.maxRetries()); + log.info("Executing deferred action '{}' (attempt {}, max retries {})", + action.actionId(), runningAction.attempts(), runningAction.maxRetries());🤖 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/deferred/DeferredActionScheduler.java` around lines 419 - 420, The log message in DeferredActionScheduler is misleading because it prints runningAction.attempts() and runningAction.maxRetries() as "attempt X/Y" even though maxRetries is a retry limit, not total attempts; update the log to either display "(attempt {}/{})" as runningAction.attempts() and (runningAction.maxRetries() + 1) to show total possible attempts, or change the message to "(attempt {} - retry {}/{})" or similar so it clearly distinguishes current attempt from retry limit; locate the log using action.actionId(), runningAction.attempts(), and runningAction.maxRetries() and adjust the formatted values/text accordingly.
🤖 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/deferred/DeferredActionScheduler.java`:
- Around line 505-518: toMessages currently assumes history is non-null and will
NPE if session.messages() returned null; update the method to guard the history
parameter by replacing direct iteration with a null-safe local like var safe =
history != null ? history : List.of() and iterate over safe, preserving existing
handling of AgentSession.ConversationMessage cases in toMessages so behavior is
unchanged when history is present.
- Around line 99-122: In DeferredActionScheduler's constructor, add null checks
using Objects.requireNonNull(...) for the critical parameters store,
sessionManager, llmClient, toolRegistry, and sessionTurnLocks before assigning
them to fields so they cannot be null when methods like tick(), executeAction(),
or schedule() run; update the DeferredActionScheduler constructor to call
Objects.requireNonNull(store, "store"), Objects.requireNonNull(sessionManager,
"sessionManager"), Objects.requireNonNull(llmClient, "llmClient"),
Objects.requireNonNull(toolRegistry, "toolRegistry"), and
Objects.requireNonNull(sessionTurnLocks, "sessionTurnLocks") and then assign the
validated values to the corresponding fields.
---
Duplicate comments:
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Around line 212-227: The idempotency dedupe (computeIdempotencyKey +
store.findByIdempotencyKey) must run before the pending-limit checks to avoid
rejecting duplicate requests; in DeferredActionScheduler move the idempotency
block (computeIdempotencyKey, var existing = store.findByIdempotencyKey(...),
logging and return of existing.get()) so it executes prior to calling
store.pendingCountForSession(...) and store.totalPendingCount(...), leaving the
limit checks intact for new actions only.
- Around line 380-388: The busy-session branch currently offers the same PENDING
action into queuedActions every tick, causing duplicate ActionQueued events and
duplicate entries; modify the enqueue logic in DeferredActionScheduler so it
deduplicates by action ID (use a concurrent Set like queuedActionIds alongside
queuedActions) — before calling queuedActions.computeIfAbsent(...).offer(action)
check-and-add the actionId into queuedActionIds (only offer if add returns true)
and still publish ActionQueued once; additionally, when draining in
notifyTurnComplete() remove the actionId from queuedActionIds as you
poll/dispatch to prevent stale IDs from blocking future re-queues.
---
Nitpick comments:
In
`@aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java`:
- Around line 261-263: The cancel method in DeferredActionScheduler currently
calls store.get(actionId) without validating actionId; add a null (and
optionally blank) check at the start of DeferredActionScheduler.cancel to
immediately return false (or throw IllegalArgumentException if project
convention prefers) when actionId is null/empty, so store.get(...) is never
invoked with a null key; update the method signature body around the existing
store.get(...) call to perform this guard before calling store.get(actionId).
- Around line 419-420: The log message in DeferredActionScheduler is misleading
because it prints runningAction.attempts() and runningAction.maxRetries() as
"attempt X/Y" even though maxRetries is a retry limit, not total attempts;
update the log to either display "(attempt {}/{})" as runningAction.attempts()
and (runningAction.maxRetries() + 1) to show total possible attempts, or change
the message to "(attempt {} - retry {}/{})" or similar so it clearly
distinguishes current attempt from retry limit; locate the log using
action.actionId(), runningAction.attempts(), and runningAction.maxRetries() and
adjust the formatted values/text accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
aceclaw-daemon/src/main/java/dev/aceclaw/daemon/deferred/DeferredActionScheduler.java
Comment on lines
+99
to
+122
| public DeferredActionScheduler( | ||
| DeferredActionStore store, | ||
| SessionManager sessionManager, | ||
| LlmClient llmClient, | ||
| ToolRegistry toolRegistry, | ||
| String model, | ||
| String systemPrompt, | ||
| int maxTokens, | ||
| int thinkingBudget, | ||
| EventBus eventBus, | ||
| int tickSeconds, | ||
| ConcurrentHashMap<String, ReentrantLock> sessionTurnLocks) { | ||
| this.store = store; | ||
| this.sessionManager = sessionManager; | ||
| this.llmClient = llmClient; | ||
| this.toolRegistry = toolRegistry; | ||
| this.model = model; | ||
| this.systemPrompt = systemPrompt; | ||
| this.maxTokens = maxTokens; | ||
| this.thinkingBudget = thinkingBudget; | ||
| this.eventBus = eventBus; | ||
| this.tickSeconds = tickSeconds > 0 ? tickSeconds : 5; | ||
| this.sessionTurnLocks = sessionTurnLocks; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add null validation for critical constructor parameters.
Several parameters are used directly without null checks: store, sessionManager, llmClient, toolRegistry, and sessionTurnLocks. A null value will cause NPEs in tick(), executeAction(), or schedule().
🛡️ Proposed fix
public DeferredActionScheduler(
DeferredActionStore store,
SessionManager sessionManager,
LlmClient llmClient,
ToolRegistry toolRegistry,
String model,
String systemPrompt,
int maxTokens,
int thinkingBudget,
EventBus eventBus,
int tickSeconds,
ConcurrentHashMap<String, ReentrantLock> sessionTurnLocks) {
- this.store = store;
- this.sessionManager = sessionManager;
- this.llmClient = llmClient;
- this.toolRegistry = toolRegistry;
+ this.store = Objects.requireNonNull(store, "store");
+ this.sessionManager = Objects.requireNonNull(sessionManager, "sessionManager");
+ this.llmClient = Objects.requireNonNull(llmClient, "llmClient");
+ this.toolRegistry = Objects.requireNonNull(toolRegistry, "toolRegistry");
this.model = model;
this.systemPrompt = systemPrompt;
this.maxTokens = maxTokens;
this.thinkingBudget = thinkingBudget;
this.eventBus = eventBus;
this.tickSeconds = tickSeconds > 0 ? tickSeconds : 5;
- this.sessionTurnLocks = sessionTurnLocks;
+ this.sessionTurnLocks = Objects.requireNonNull(sessionTurnLocks, "sessionTurnLocks");
}As per coding guidelines: "Use Objects.requireNonNull(param, \"param\") on parameters used in .equals() or passed to downstream calls".
🤖 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/deferred/DeferredActionScheduler.java`
around lines 99 - 122, In DeferredActionScheduler's constructor, add null checks
using Objects.requireNonNull(...) for the critical parameters store,
sessionManager, llmClient, toolRegistry, and sessionTurnLocks before assigning
them to fields so they cannot be null when methods like tick(), executeAction(),
or schedule() run; update the DeferredActionScheduler constructor to call
Objects.requireNonNull(store, "store"), Objects.requireNonNull(sessionManager,
"sessionManager"), Objects.requireNonNull(llmClient, "llmClient"),
Objects.requireNonNull(toolRegistry, "toolRegistry"), and
Objects.requireNonNull(sessionTurnLocks, "sessionTurnLocks") and then assign the
validated values to the corresponding fields.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
xinhuagu
force-pushed
the
feat/issue-141-deferred-action
branch
from
08bcc7b to
2b9f203
Compare
xinhuagu
dismissed stale reviews from coderabbitai[bot], coderabbitai[bot], coderabbitai[bot], and coderabbitai[bot]
All findings addressed in subsequent commits
added 6 commits
March 1, 2026 21:58
…ions (#141) Add a deferred action system that allows the agent to schedule one-shot delayed checks. The scheduler wakes the session via system message injection and runs the check on a virtual thread. Results are streamed to the CLI as foreground notifications (printed above the prompt), mirroring the existing cron notification pipeline. New files: - DeferEvent sealed interface (7 lifecycle event records) - DeferredAction record, DeferredActionState enum - DeferredActionStore (JSON file persistence) - DeferredActionScheduler (tick-based, per-session turn lock) - DeferredEventFeed (ring buffer for event polling) - DeferCheckTool (agent-facing tool) - DeferredActionSchedulerTest (26 tests) Modified: - AceClawEvent: add DeferEvent to permits - StreamingAgentHandler: per-session ReentrantLock, notifyTurnComplete hook - AceClawDaemon: wiring, EventBus subscription, RPC routes - AceClawConfig: deferredActionEnabled, deferredActionTickSeconds - TerminalRepl: deferred event polling + ANSI rendering
- Critical: executeAction() now acquires session turn lock to prevent race condition with handlePrompt() on concurrent session access - Major: move startMonitor() inside try block to prevent lock leak if it throws - Major: sanitize goal/error/reason fields before terminal rendering to prevent control character injection - Minor: add Objects.requireNonNull for sessionId/goal in schedule() - Minor: add null-guards in DeferredActionStore query methods - Minor: move sequence increment inside synchronized in DeferredEventFeed - Nitpick: remove unused Objects import in DeferCheckTool - Nitpick: null-guard List in PollResult record constructor
- notifyTurnComplete drained threads now acquire session turn lock before executing (same pattern as tick virtual threads) - Clean up empty queuedActions entries after drain to prevent leak - Clear queuedActions on stop()
- Wrap Thread.ofVirtual().start() in tick() with try/finally to guarantee turnLock.unlock() even if thread creation fails - Track active virtual worker threads via activeWorkers Set for graceful shutdown coordination - Await in-flight workers in stop() with 15s timeout before clearing state - Self-remove from activeWorkers in executeAction() via outer try/finally Addresses CodeRabbit second-round review findings.
…ash guard - Move idempotency check before limit checks in schedule() so duplicates are deduped instead of failing with "limit reached" - Add queuedActionIds dedup set to prevent same action being re-enqueued every tick while session is busy - Guard substring(0, 16) on SHA-256 hash with length check per coding guidelines
… clarity - Add Objects.requireNonNull for store, sessionManager, sessionTurnLocks in constructor (llmClient/toolRegistry nullable for scheduling-only usage) - Add null check for actionId in cancel() - Null-guard history list in toMessages() - Clarify log message: "attempt X, max retries Y" instead of "attempt X/Y"
xinhuagu
force-pushed
the
feat/issue-141-deferred-action
branch
from
2b9f203 to
98c140c
Compare
This was referenced Mar 1, 2026
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
defer_checktoolDeferredActionSchedulerticks every 5s, wakes idle sessions via system message injection, and runs checks on virtual threadsReentrantLockcoordinates turn concurrency — scheduler queues actions if a session is busy, drains onnotifyTurnCompleteNew Files (8)
DeferEvent.javaDeferredActionState.javaDeferredAction.javaDeferredActionStore.java~/.aceclaw/deferred/actions.json), ReadWriteLock + atomic writeDeferredActionScheduler.javaDeferredEventFeed.javaDeferCheckTool.javadefer_check(delaySeconds, goal, maxRetries?)DeferredActionSchedulerTest.javaModified Files (5)
AceClawEvent.javaDeferEventto sealed permitsStreamingAgentHandler.javaReentrantLock,notifyTurnCompletehook, permission levels forcron/defer_checkAceClawDaemon.javadeferred.events.poll+deferred.status+deferred.cancelRPC routesAceClawConfig.javadeferredActionEnabled(default true),deferredActionTickSeconds(default 5)TerminalRepl.javaTest plan
./gradlew clean build— 360/362 tests pass (2 pre-existing failures)defer_checkwith 10s delay, verify CLI shows[deferred running]and--- deferred completed ---notifications above promptCloses #141
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Configuration
Tests