feat(core): add abort controller & implement waitUntil

[NathanFlurry]

No description provided.

[claude]

Claude finished @NathanFlurry's task —— View job

---

Code Review: feat(core): add abort controller & implement waitUntil

Summary:
This PR implements abort controller functionality and renames/improves the background task management system from runInBackground to waitUntil. The changes enhance graceful shutdown handling and provide better control over background task lifecycle.

---

✅ Positive Aspects

1. Clear Naming & Documentation

• The rename from runInBackground to waitUntil is more descriptive and aligns with standard Web API naming
• Good documentation clarifying the purpose: "Prevents the actor from sleeping until promise is complete"
• Added helpful configuration comment for waitUntilTimeout

2. Robust Shutdown Handling

• Added proper timeout handling for background promises during shutdown (#waitBackgroundPromises)
• AbortController provides clean cancellation mechanism for listeners
• Enhanced #assertReady() to check for sleep/stop states preventing race conditions

3. Consistent Error Handling

• Background promises are wrapped to be non-failable with proper logging
• Graceful handling of timeout scenarios with detailed error messages

---

⚠️ Issues & Concerns

1. Code Quality Issues

Missing Error Handling in AbortController:

// packages/core/src/actor/instance.ts:1589-1591
try {
    this.#abortController.abort();
} catch {}

Empty catch blocks should log or document why errors are ignored. Consider:

try {
    this.#abortController.abort();
} catch (error) {
    logger().debug("abort controller already aborted", { error: stringifyError(error) });
}

2. Potential Race Condition
The #assertReady() method now throws when #sleepCalled or #stopCalled is true, but there could be timing issues where a waitUntil call is made during the brief window between these flags being set and the actual shutdown:

// In _waitUntil method - packages/core/src/actor/instance.ts:1458
_waitUntil(promise: Promise<void>) {
    this.#assertReady(); // Could throw during shutdown window
    // ... rest of method
}

3. Configuration Validation
The new waitUntilTimeout config has a reasonable default (15 seconds), but there's no validation that it's less than other timeouts that depend on it.

4. Memory Management
Background promises are added to an array but never cleaned up until shutdown. For long-running actors with many waitUntil calls, this could cause memory growth. Consider cleaning up completed promises.

---

🔒 Security Considerations

• Low Risk: AbortController usage is safe and follows standard patterns
• The timeout mechanisms prevent DoS scenarios from hanging background tasks

---

⚡ Performance Considerations

• Memory Growth: #backgroundPromises array grows over time and is never pruned
• Promise.allSettled: During shutdown, waits for ALL background promises rather than allowing early completion
• Consider implementing a cleanup mechanism for resolved promises

---

📋 Test Coverage

Missing Tests for:

• AbortSignal behavior during shutdown scenarios
• waitUntil timeout handling during actor shutdown
• Race conditions between waitUntil calls and sleep/stop states
• Memory behavior with many waitUntil calls over time

---

🛠️ Recommendations

1. Improve Error Handling:

// Add proper logging to abort controller
try {
    this.#abortController.abort();
} catch (error) {
    logger().warn("failed to abort controller", { error: stringifyError(error) });
}

2. Add Background Promise Cleanup:

// Clean up resolved promises periodically
#cleanupBackgroundPromises() {
    this.#backgroundPromises = this.#backgroundPromises.filter(p => 
        // Keep only pending promises (this is tricky to implement correctly)
    );
}

3. Consider Configuration Validation:

// In config validation, ensure waitUntilTimeout < onStopTimeout + buffer

4. Add Integration Tests:

• Test abort signal behavior in real shutdown scenarios
• Test waitUntil with various timing combinations
• Test memory behavior over extended periods

---

📊 Overall Assessment

Code Quality: B+ - Well-implemented with good naming and documentation, but missing some error handling details

Architecture: A- - Clean separation of concerns and follows existing patterns well

Safety: B - Generally safe but could use better race condition handling

This is a solid implementation that significantly improves the actor shutdown lifecycle. The rename to waitUntil makes the API more intuitive, and the addition of AbortController provides proper cancellation primitives. The main concerns are around edge case handling and long-term memory management.

Recommendation: ✅ Approve with minor fixes

The issues identified are not blocking, but would benefit from follow-up improvements, particularly around error logging and memory management.

---

[NathanFlurry]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• chore: simplify package directories #1205
• chore(core): migrate actor persist & file system to bare #1204
• chore(core): migrate client protocol to BARE #1203
• chore(core): move actor liveness check to setInterval #1202
• fix(core): fix disconnecting sse connections #1201
• feat(core): add abort controller & implement waitUntil #1200 👈 (View in Graphite)
• feat(core): actor sleeping #1178
• fix(core): revert deleted fixtures #1199
• main

---

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

• merge queue - adds this PR to the back of the merge queue
• hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[graphite-app]

It appears that the properties #sleepCalled and #stopCalled are being checked in the #assertReady() method, but they don't seem to be defined as class properties in the visible code. These properties need to be declared at the class level (similar to how #backgroundPromises and other private fields are declared) and properly initialized to avoid potential runtime errors.

Consider adding these property declarations to the class, for example:

#sleepCalled = false;
#stopCalled = false;

Then ensure they're properly set to true in the respective _sleep() and _stop() methods to maintain accurate lifecycle state tracking.

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

Potential issues with #waitBackgroundPromises implementation:

1. Race condition with promise collection: The method captures this.#backgroundPromises at the beginning, but new promises could be added via _waitUntil() during execution. This means some promises might not be properly awaited during shutdown.

2. Timer resource leak: The setTimeout in the race condition creates a timer that isn't explicitly cleared if promises resolve first. Consider storing the timeout ID and clearing it when no longer needed:

const timeoutId = setTimeout(() => resolve(true), timeoutMs);
Promise.allSettled(pending).then(() => {
  clearTimeout(timeoutId);
  resolve(false);
});

3. Promise array management: There doesn't appear to be a mechanism to remove completed promises from #backgroundPromises. Without cleanup, this array will grow indefinitely during the actor's lifetime, potentially causing memory issues.

Consider implementing a promise tracking system that removes completed promises and properly handles the race condition during shutdown.

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[graphite-app]

The abort() method is wrapped in an empty try-catch block that suppresses all errors without logging. While AbortController.abort() rarely throws, silent error handling can hide unexpected issues during the critical shutdown sequence. Consider either:

1. Removing the try-catch if abort() is not expected to throw in your implementation
2. Adding error logging inside the catch block
3. Catching only specific expected exceptions

This would improve debuggability if unexpected behavior occurs during shutdown.

Suggested change

	try {
	this.#abortController.abort();
	} catch {}
	try {
	this.#abortController.abort();
	} catch (error) {
	console.error("Error aborting actor controller:", error);
	}

Spotted by Diamond



Is this helpful? React 👍 or 👎 to let us know.

[claude]

Claude finished @NathanFlurry's task —— View job

---

PR Review: feat(core): add abort controller & implement waitUntil

I'll review this PR focusing on:

• Code quality and best practices
• Potential bugs or issues
• Performance considerations
• Security concerns
• Test coverage

Summary

This PR implements abort controller functionality and renames runInBackground to waitUntil for better background task management during actor shutdown. The changes enhance graceful shutdown handling and provide better control over background task lifecycle.

---

✅ Strengths

1. Improved API Design

• waitUntil is more descriptive than runInBackground and aligns with Web API naming conventions
• Clear documentation: "Prevents the actor from sleeping until promise is complete"
• Good JSDoc comments explaining the purpose and timeout behavior

2. Enhanced Shutdown Logic

• AbortController provides standard cancellation mechanism for listeners
• Proper timeout handling for background promises during shutdown
• Enhanced #assertReady() prevents race conditions during state transitions

3. Configuration Management

• waitUntilTimeout has a reasonable default (15 seconds)
• Clear comment explaining the background promise timeout purpose

---

⚠️ Critical Issues

1. Missing Class Properties
The Diamond AI review correctly identified that #sleepCalled and #stopCalled properties are referenced but not declared. However, I can see these are properly declared at lines 135-136 in instance.ts:

#sleepCalled = false;
#stopCalled = false;

This appears to be a false positive from the Diamond AI review.

2. Silent Error Handling
In packages/core/src/actor/instance.ts:1592-1594:

try {
    this.#abortController.abort();
} catch {}

Empty catch blocks should be avoided. Per the repository's logging guidelines, this should use structured logging:

try {
    this.#abortController.abort();
} catch (error) {
    logger().debug("abort controller already aborted", { error: stringifyError(error) });
}

3. Memory Leak Potential
Background promises are accumulated in #backgroundPromises array but never cleaned up until shutdown. For long-running actors with frequent waitUntil calls, this causes unbounded memory growth.

Recommendation: Implement periodic cleanup of resolved promises or use a different tracking mechanism.

4. Race Condition Risk
In #waitBackgroundPromises() (lines 1651-1677), the method captures the promise array at the start, but new promises can be added during execution via _waitUntil(). This means promises added during shutdown might not be properly awaited.

---

🔒 Security Considerations

• Low Risk: AbortController usage follows secure patterns
• Timeout mechanisms prevent DoS scenarios from hanging background tasks
• No sensitive data exposure risks identified

---

⚡ Performance Issues

1. Memory Growth

• #backgroundPromises array grows indefinitely during actor lifetime
• No cleanup mechanism for resolved promises until shutdown

2. Promise.allSettled Overhead

• During shutdown, waits for ALL background promises rather than allowing early completion
• Consider allowing configurable early termination strategies

3. Timer Resource Management
The timeout in #waitBackgroundPromises creates a timer that isn't explicitly cleared:

setTimeout(() => resolve(true), timeoutMs)

Should store timeout ID and clear it when promises resolve first.

---

🧪 Test Coverage Gaps

The implementation lacks tests for critical scenarios:

• AbortSignal behavior during various shutdown scenarios
• waitUntil timeout handling during actor shutdown
• Race conditions between waitUntil calls and sleep/stop states
• Memory behavior with many waitUntil calls over time
• Edge cases where promises are added during shutdown

---

📋 Specific Recommendations

1. Fix Silent Error Handling (High Priority)

// In _stop() method around line 1593
try {
    this.#abortController.abort();
} catch (error) {
    logger().debug("abort controller already aborted", { error: stringifyError(error) });
}

2. Add Background Promise Cleanup (Medium Priority)

#cleanupResolvedPromises() {
    // Implementation would need to track promise states
    // This is complex to implement correctly without Promise.allSettled
}

3. Fix Timer Resource Leak (Medium Priority)

async #waitBackgroundPromises(timeoutMs: number) {
    const pending = this.#backgroundPromises;
    if (pending.length === 0) return;

    const timeoutId = setTimeout(() => resolve(true), timeoutMs);
    const timedOut = await Promise.race([
        Promise.allSettled(pending).then(() => {
            clearTimeout(timeoutId);
            return false;
        }),
        new Promise<true>((resolve) => {
            timeoutId = setTimeout(() => resolve(true), timeoutMs);
        })
    ]);
}

4. Add Configuration Validation
Ensure waitUntilTimeout < onStopTimeout to prevent conflicts during shutdown.

---

📊 Overall Assessment

Code Quality: B+ - Well-implemented with good naming, but needs error handling fixes

Architecture: A- - Clean design following existing patterns, good separation of concerns

Safety: B - Generally safe but needs better race condition and resource management

Performance: C+ - Memory leaks and resource management issues need addressing

This is a solid implementation that significantly improves the actor shutdown lifecycle. The API rename makes the functionality more intuitive, and the AbortController addition provides proper cancellation primitives.

Recommendation: ✅ Approve with minor fixes required

The critical issues around error handling and memory management should be addressed, but they don't block the core functionality. The implementation provides valuable improvements to actor lifecycle management.

---

[graphite-app]

Merge activity

• Sep 11, 10:34 PM UTC: NathanFlurry added this pull request to the Graphite merge queue.
• Sep 11, 10:35 PM UTC: CI is running for this pull request on a draft pull request (#1206) due to your merge queue CI optimization settings.
• Sep 11, 10:35 PM UTC: Merged by the Graphite merge queue via draft PR: #1206.