Skip to content

Commit ad61247

Browse files
KrisBraunclaude
andcommitted
connector-apple-calendar: harden polling chain + write-ordering
Address review feedback on ace1549: 1. Catch paths in syncBatch and syncBatchContinue now schedulePoll before re-throwing. Previously, if startIncrementalSync bailed on lock contention while the active holder caught an exception, polling stopped permanently — no path scheduled the next poll. initChannel's lock-fail bail also schedules a recovery poll. 2. Etag and uid maps now written together at end-of-batch in processCalDAVEvents, so a worker crash mid-batch can't leave etags_* claiming events that have no uid mapping (which would silently drop their future deletions as "missingUid" forever). 3. Catch-block cleanup wrapped in its own try/catch so a failed releaseLock/clear doesn't mask the original error. Also: acknowledge the bulk-delete archiveLinks-loop scalability with a comment (future work to add a multi-uid filter); update the now- stale "future selective deletion" comment to describe current behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent ace1549 commit ad61247

1 file changed

Lines changed: 86 additions & 26 deletions

File tree

‎connectors/apple-calendar/src/apple-calendar.ts‎

Lines changed: 86 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,11 @@ export class AppleCalendar extends Connector<AppleCalendar> {
233233
AppleCalendar.SYNC_LOCK_TTL_MS
234234
);
235235
if (!acquired) {
236+
// Another sync holds the lock (e.g. an in-flight init from a previous
237+
// enable attempt that hasn't drained, or a stuck TTL'd run). Schedule
238+
// a poll so the next interval can retry init — otherwise this channel
239+
// would be stuck with no scheduled work until the user re-enables.
240+
await this.schedulePoll(channelId);
236241
return;
237242
}
238243

@@ -320,17 +325,11 @@ export class AppleCalendar extends Connector<AppleCalendar> {
320325
end: timeRangeEnd,
321326
});
322327

323-
// Store etags for incremental sync
324-
const etagMap: Record<string, string> = {};
325-
for (const event of events) {
326-
etagMap[event.href] = event.etag;
327-
}
328-
await this.set(`etags_${calendarHref}`, etagMap);
329-
330-
// Process events in batches. processCalDAVEvents records each
331-
// event's href→uid into event_uids_<calendarHref> so future
332-
// selective per-event deletion (see startIncrementalSync) can
333-
// resolve a deleted href back to its uid.
328+
// Process events in batches. processCalDAVEvents persists the
329+
// href→uid AND href→etag maps together at end-of-batch so the two
330+
// stay consistent across worker crashes — see the comment on
331+
// processCalDAVEvents for why we deliberately don't pre-write
332+
// etags before processing.
334333
await this.processCalDAVEvents(
335334
events.slice(0, 50),
336335
calendarHref,
@@ -365,9 +364,23 @@ export class AppleCalendar extends Connector<AppleCalendar> {
365364
);
366365

367366
// Release lock and clear state so future syncs aren't permanently
368-
// blocked. Even if this release fails, the lock's TTL will expire it.
369-
await this.tools.store.releaseLock(`sync_${calendarHref}`);
370-
await this.clear(`sync_state_${calendarHref}`);
367+
// blocked. Wrap in its own try/catch so a release/clear failure
368+
// doesn't mask the original error — the lock's TTL is the safety net.
369+
try {
370+
await this.tools.store.releaseLock(`sync_${calendarHref}`);
371+
await this.clear(`sync_state_${calendarHref}`);
372+
} catch (cleanupError) {
373+
console.error(
374+
`Apple Calendar sync cleanup after failure also failed for ${calendarHref}:`,
375+
cleanupError
376+
);
377+
}
378+
379+
// Schedule a poll so polling resumes — otherwise a failure here
380+
// strands the channel (startIncrementalSync's lock-fail bail
381+
// intentionally relies on the active holder, which is us, to
382+
// reschedule).
383+
await this.schedulePoll(calendarHref);
371384

372385
// Re-throw to let the runtime handle it (PostHog capture, etc.).
373386
throw error;
@@ -421,9 +434,21 @@ export class AppleCalendar extends Connector<AppleCalendar> {
421434
);
422435

423436
// Release lock and clear state so future syncs aren't permanently
424-
// blocked. Even if this release fails, the lock's TTL will expire it.
425-
await this.tools.store.releaseLock(`sync_${calendarHref}`);
426-
await this.clear(`sync_state_${calendarHref}`);
437+
// blocked. Wrap cleanup so a release/clear failure doesn't mask the
438+
// original error — the lock's TTL is the safety net.
439+
try {
440+
await this.tools.store.releaseLock(`sync_${calendarHref}`);
441+
await this.clear(`sync_state_${calendarHref}`);
442+
} catch (cleanupError) {
443+
console.error(
444+
`Apple Calendar sync cleanup after failure also failed for ${calendarHref}:`,
445+
cleanupError
446+
);
447+
}
448+
449+
// Schedule a poll so polling resumes — startIncrementalSync's
450+
// lock-fail bail relies on the active holder (us) to reschedule.
451+
await this.schedulePoll(calendarHref);
427452

428453
throw error;
429454
}
@@ -571,6 +596,11 @@ export class AppleCalendar extends Connector<AppleCalendar> {
571596
// channel without the data-loss risk above.
572597
let archivedCount = 0;
573598
let missingUidCount = 0;
599+
// Per-uid archive is serial — fine for typical incremental drift
600+
// (≤handful of deletes per poll), but a bulk delete (user clearing
601+
// a multi-year backfill) could approach the ~1000-request runtime
602+
// limit. If that becomes a real problem, extend archiveLinks to
603+
// accept a uids[] filter and batch.
574604
for (const href of deletedHrefs) {
575605
const uid = storedUids[href];
576606
if (!uid) {
@@ -628,9 +658,17 @@ export class AppleCalendar extends Connector<AppleCalendar> {
628658
error
629659
);
630660

631-
// Release lock so future syncs aren't permanently blocked. The TTL
632-
// is the safety net if this release also fails.
633-
await this.tools.store.releaseLock(`sync_${calendarHref}`);
661+
// Release lock so future syncs aren't permanently blocked. Wrap in
662+
// its own try/catch so a release failure doesn't mask the real
663+
// error — the lock's TTL is the safety net.
664+
try {
665+
await this.tools.store.releaseLock(`sync_${calendarHref}`);
666+
} catch (cleanupError) {
667+
console.error(
668+
`Apple Calendar incremental sync cleanup after failure also failed for ${calendarHref}:`,
669+
cleanupError
670+
);
671+
}
634672

635673
// Reschedule a poll so we recover on the next interval.
636674
await this.schedulePoll(calendarHref);
@@ -644,9 +682,16 @@ export class AppleCalendar extends Connector<AppleCalendar> {
644682
/**
645683
* Process CalDAV events (parse ICS and save as links).
646684
*
647-
* Also maintains the `event_uids_<calendarHref>` map keyed by event href
648-
* so future incremental syncs can archive deleted events selectively by
649-
* uid instead of wiping the whole channel (see startIncrementalSync).
685+
* Also maintains the `event_uids_<calendarHref>` and `etags_<calendarHref>`
686+
* maps keyed by event href so future incremental syncs can archive deleted
687+
* events selectively by uid (see startIncrementalSync). Both maps are
688+
* updated together at end-of-batch as one logical commit per batch — if a
689+
* worker crashes mid-batch and never reaches this point, neither map is
690+
* advanced, keeping stored etags and stored uids consistent. Writing etags
691+
* before this method ran would have stranded hrefs in the etag set with no
692+
* uid mapping, so a future deletion would silently drop (logged as
693+
* missingUid).
694+
*
650695
* Recurrence-only entries (RECURRENCE-ID overrides) share the same uid
651696
* as their master, so the master entry already covers them — we record
652697
* uid once per href.
@@ -656,16 +701,28 @@ export class AppleCalendar extends Connector<AppleCalendar> {
656701
calendarHref: string,
657702
initialSync: boolean
658703
): Promise<void> {
659-
// Load the persisted href→uid map once, merge new entries from this
660-
// batch in memory, and write back once at the end. Avoids one
661-
// read+write per event.
704+
// Load persisted href→uid and href→etag maps once, merge new entries
705+
// from this batch in memory, and write back together at the end. Avoids
706+
// one read+write per event and ensures both maps advance atomically per
707+
// batch.
662708
const uidMap =
663709
(await this.get<Record<string, string>>(
664710
`event_uids_${calendarHref}`
665711
)) || {};
712+
const etagMap =
713+
(await this.get<Record<string, string>>(`etags_${calendarHref}`)) || {};
666714
let uidMapDirty = false;
715+
let etagMapDirty = false;
667716

668717
for (const caldavEvent of events) {
718+
// Record the etag up-front for every event in this batch so the
719+
// stored set matches the uids we're about to record. Both maps are
720+
// persisted together below.
721+
if (etagMap[caldavEvent.href] !== caldavEvent.etag) {
722+
etagMap[caldavEvent.href] = caldavEvent.etag;
723+
etagMapDirty = true;
724+
}
725+
669726
try {
670727
const icsEvents = parseICSEvents(caldavEvent.icsData);
671728

@@ -705,6 +762,9 @@ export class AppleCalendar extends Connector<AppleCalendar> {
705762
if (uidMapDirty) {
706763
await this.set(`event_uids_${calendarHref}`, uidMap);
707764
}
765+
if (etagMapDirty) {
766+
await this.set(`etags_${calendarHref}`, etagMap);
767+
}
708768
}
709769

710770
/**

0 commit comments

Comments
 (0)