Skip to content

fix(store): bound EventKit fetchReminders with a 30s timeout - #66

Open
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/eventkit-fetch-timeout
Open

fix(store): bound EventKit fetchReminders with a 30s timeout#66
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/eventkit-fetch-timeout

Conversation

@SebTardif

Copy link
Copy Markdown

What Problem This Solves

fetchReminders used withCheckedContinuation with no timeout. If EventKit never invokes the callback, remindctl list (and any path that loads reminders) hangs forever.

Evidence

After

  • Throwing continuation with one-shot resume helper
  • Parallel Task.sleep 30s resumes operationFailed("Timed out waiting for EventKit reminders after 30s")
  • Late EventKit callbacks ignored after first resume

Real behavior proof

  • Behavior or issue addressed: Bound EventKit reminder fetch so list cannot hang indefinitely.
  • Real environment tested: macOS arm64, worktree /tmp/oc-batch-20260802/remindctl.
  • Exact steps or command run after this patch: Code review of timeout path; compile via commit hooks if present.
  • Evidence after fix: fetchReminders is async throws with 30s bound; public reminders(matching:) uses try await.
  • Observed result after fix: Timeout maps to RemindCoreError.operationFailed.
  • What was not tested: Live EventKit stall (requires mocking/stuck EKEventStore); full Xcode test suite not run in this session.

Summary

30s timeout around EventKit fetchReminders with safe one-shot resume.

fetchReminders used an unbounded continuation. If EventKit never calls
back, remindctl list hangs forever. Resume once with a 30s timeout and
map the failure to operationFailed.

Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Aug 2, 2026
@clawsweeper

clawsweeper Bot commented Aug 2, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 2, 2026, 8:37 AM ET / 12:37 UTC.

ClawSweeper review

What this changes

The PR changes EventKit reminder retrieval to throw after 30 seconds rather than letting reminder-loading commands wait indefinitely for an EventKit callback.

Merge readiness

Blocked until stronger real behavior proof is added - 4 items remain

This PR remains necessary: current main still waits on an EventKit continuation with no timeout, so a missing callback can block reminder-loading commands indefinitely. The timeout direction is focused and source-consistent, but the PR needs after-fix runtime evidence before merge because its body documents code review rather than an observed EventKit result.

Priority: P2
Reviewed head: db8073ef8403a3bb07abc5168e531ffdf0242745

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The focused patch is plausible, but its runtime behavior and timeout policy are not yet demonstrated with real after-fix evidence.
Proof confidence 🧂 unranked krab (1/6) Needs stronger real behavior proof before merge: The PR explicitly reports code review and no live EventKit-stall run, so it does not show an observed after-fix result. Please add redacted terminal output, runtime logs, or a reproducible local harness result; after updating the PR body, request @clawsweeper re-review if a fresh review does not start automatically. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The PR explicitly reports code review and no live EventKit-stall run, so it does not show an observed after-fix result. Please add redacted terminal output, runtime logs, or a reproducible local harness result; after updating the PR body, request @clawsweeper re-review if a fresh review does not start automatically. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Current main has no fetch bound: The current implementation awaits a checked continuation whose only resume occurs inside EventKit’s callback, with no timeout or cancellation completion path.
CLI paths await this API: The list command requests access and then awaits RemindersStore.reminders, so an unanswered EventKit callback prevents command completion.
Proposed scope: The branch changes one source file, making fetchReminders throwing, adding a lock-protected one-shot continuation, and routing timeout failure through the existing public error type.
Findings None None.
Security None None.

How this fits together

RemindersStore is the EventKit-backed core used by the CLI’s list, search, delete, and info flows. Those commands select calendars, retrieve reminders asynchronously, then render results or act on the returned records.

flowchart LR
  A[CLI reminder commands] --> B[Reminders store]
  B --> C[Calendar selection]
  C --> D[EventKit reminder fetch]
  D --> E[Callback or timeout]
  E --> F[Reminder records or error]
  F --> G[CLI output or command action]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The PR explicitly reports code review and no live EventKit-stall run, so it does not show an observed after-fix result. Please add redacted terminal output, runtime logs, or a reproducible local harness result; after updating the PR body, request @clawsweeper re-review if a fresh review does not start automatically. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - A legitimate EventKit callback that takes longer than 30 seconds will now surface operationFailed instead of continuing to wait, and the PR does not yet show runtime evidence for that tradeoff or normal callback completion.
  • Resolve merge risk (P2) - The unstructured timeout task remains alive until its sleep completes even when EventKit returns first; it is guarded against double-resume, but its behavior has not been exercised in a real run.
  • Complete next step (P2) - This needs contributor-supplied real behavior proof and ordinary maintainer review, not an automated repair PR.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
EventKit retrieval change 1 file changed; 41 added, 4 removed The full patch concentrates on the central callback boundary used by multiple reminder commands.

Merge-risk options

Maintainer options:

  1. Add runtime evidence before merge (recommended)
    Provide redacted after-fix terminal or runtime evidence for normal EventKit retrieval plus a focused timeout-path demonstration, then refresh review.
  2. Pause for a narrower timeout design
    If a 30-second failure for slow EventKit responses is not acceptable, pause this PR and agree on the timeout policy before changing the core fetch behavior.

Technical review

Best possible solution:

Land a bounded, single-resume fetch only after proving that normal EventKit retrieval still succeeds and that a deliberately stalled or controllably delayed retrieval reports the intended timeout without a double-resume failure.

Do we have a high-confidence way to reproduce the issue?

Yes, from current source: fetchReminders has exactly one completion path, EventKit’s callback, so an absent callback leaves the awaiting command unresolved. The PR does not demonstrate a live stalled callback on macOS.

Is this the best way to solve the issue?

Unclear: the timeout plus one-shot continuation is a narrow maintainable remedy, but its selected 30-second policy and post-fix behavior still need runtime proof before it is confirmed as the best final implementation.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning high; reviewed against f6ec31cf074a.

Labels

Label changes:

  • add P2: An unreturned EventKit callback blocks normal reminder command completion, but the report does not establish an active broad outage.
  • add merge-risk: 🚨 availability: The patch changes whether the core EventKit fetch waits indefinitely or fails after 30 seconds.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR explicitly reports code review and no live EventKit-stall run, so it does not show an observed after-fix result. Please add redacted terminal output, runtime logs, or a reproducible local harness result; after updating the PR body, request @clawsweeper re-review if a fresh review does not start automatically. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: An unreturned EventKit callback blocks normal reminder command completion, but the report does not establish an active broad outage.
  • merge-risk: 🚨 availability: The patch changes whether the core EventKit fetch waits indefinitely or fails after 30 seconds.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR explicitly reports code review and no live EventKit-stall run, so it does not show an observed after-fix result. Please add redacted terminal output, runtime logs, or a reproducible local harness result; after updating the PR body, request @clawsweeper re-review if a fresh review does not start automatically. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • Peter Steinberger: History shows repeated ownership of the reminder workflow, including the initial CLI implementation and the latest substantive EventKit store update. (role: feature author and recent area contributor; confidence: high; commits: d8f95101127d, ac09a1ca0c61; files: Sources/RemindCore/EventKitStore.swift, Sources/remindctl/Commands/ListCommand.swift)
  • Jeremy Lahners: History associates this contributor with a merged EventKit reminder-data change in the same store. (role: adjacent area contributor; confidence: medium; commits: 71c1e1739738; files: Sources/RemindCore/EventKitStore.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add redacted runtime proof showing ordinary EventKit retrieval still completes after the patch.
  • Provide a focused timeout-path demonstration showing the intended operationFailed result and no double-resume failure.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

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

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant