feat: adds semi-Go-like Context with irrecoverable error handling

[staheri14]

Closes #52

This PR introduces a ThrowableContext that provides Go's context.Context API (almost) patterns in Rust:

Core Features Added:

• Go-style context creation: with_cancel(), with_timeout(), with_value()
• Hierarchical context chains with automatic cancellation propagation from parent to child
• Type-safe value storage and retrieval across context boundaries
• Async cancellation and timeout support integrated with tokio runtime
• Irrecoverable error propagation: throw_irrecoverable() terminates the program when critical errors occur

[claude]

Claude finished @staheri14's task —— View job

---

Code Review Progress

Reviewing PR #64: "feat: adds semi-Go-like Context with irrecoverable error handling"

Todo List:

• Read repository guidelines (CLAUDE.md)
• Analyze changed files and understand the implementation
• Review code quality and best practices
• Check for potential bugs and issues
• Assess performance considerations
• Evaluate security concerns
• Review test coverage
• Provide comprehensive feedback

---

Code Review Feedback

✅ Positive Aspects

1. Good Go-like API Design: The implementation successfully mirrors Go's context patterns with with_cancel(), with_timeout(), and with_value() methods.

2. Proper Documentation: Excellent module-level documentation and comprehensive method documentation explaining the irrecoverable error propagation behavior.

3. Strong Test Coverage: Good test coverage with tests for cancellation, timeout, values, and the with_cancel helper.

4. Type-Safe Value Storage: Smart use of TypeId for type-safe value storage and retrieval.

5. Examples Module: The examples.rs file provides good usage patterns and demonstrates practical scenarios.

⚠️ Critical Issues

1. Severe Violation of Repository Design Patterns

Priority: Critical

The ThrowableContext implementation completely violates the repository's core design patterns outlined in CLAUDE.md:

❌ Missing Internal Thread Safety Pattern

• Issue: The struct lacks internal thread safety using Arc<RwLock<T>>
• Expected Pattern: Should use Arc<RwLock<InnerThrowableContext>>
• Current: Direct field access without proper synchronization
• Impact: Not thread-safe for concurrent usage in a distributed system

❌ Missing Shallow Clone Pattern

• Issue: No proper shallow cloning implementation for logic-handling structures
• Expected: Clone should share underlying data via Arc
• Current: No explicit clone implementation following repo patterns
• Impact: Violates the "shallow cloning for logic-handling structures" principle

Recommended Fix:

pub struct ThrowableContext {
    inner: Arc<RwLock<InnerThrowableContext>>,
    span: Span,
}

struct InnerThrowableContext {
    token: CancellationToken,
    deadline: Option<Instant>,
    values: Arc<RwLock<HashMap<TypeId, Box<dyn Any + Send + Sync>>>>,
    parent: Option<ThrowableContext>,
}

impl Clone for ThrowableContext {
    fn clone(&self) -> Self {
        ThrowableContext {
            inner: Arc::clone(&self.inner),
            span: self.span.clone(),
        }
    }
}

2. Error Message Case Violations

Priority: High

Multiple violations of the "lowercase error messages" requirement:

• Line 186: "context cancelled" ✅ (correct)
• Line 188: "context deadline exceeded" ✅ (correct)
• Line 208: "context deadline exceeded" ✅ (correct)
• Line 218: "context cancelled" ✅ (correct)

Actually, the error messages are correctly lowercase. Good adherence to style guidelines.

3. Memory Leak in Context Hierarchy

Priority: High

Issue: src/core/context/mod.rs:85

parent: Some(self.clone()),

Problem: Creates circular references between parent and child contexts. Each child holds a reference to its parent, but if the parent also maintains references to children, this creates cycles that prevent garbage collection.

Impact: Memory leaks in long-running applications with many context hierarchies.

Suggested Fix: Use Weak<ContextInner> for parent references or implement proper lifecycle management.

4. Process Termination Concerns

Priority: High

Issue: src/core/context/mod.rs:131

std::process::exit(1);

Concerns:

• No cleanup: Abrupt termination doesn't allow for graceful shutdown
• Testing problems: Makes unit testing difficult
• Integration issues: May interfere with test frameworks or embedders

Recommendation: Consider alternatives like:

• Setting a global flag and allowing normal shutdown
• Using a configurable termination strategy
• Providing hooks for cleanup before termination

5. Lock Unwrap Anti-Pattern

Priority: Medium

Issue: Multiple unwrap() calls on lock acquisitions:

• src/core/context/mod.rs:96: values.write().unwrap()
• src/core/context/mod.rs:168: self.inner.values.read().unwrap()

Problem: Can panic if lock is poisoned, making the system less robust.

Recommended Fix: Use proper error handling:

let mut values_guard = values.write()
    .map_err(|_| anyhow!("failed to acquire write lock on context values"))?;

🔧 Performance Considerations

1. Values HashMap Cloning: The value() method clones the entire value. For large types, consider using Arc<T> within the value store.

2. Lock Contention: With the current design, the values HashMap could become a bottleneck under high concurrency. Consider using more granular locking or lock-free approaches.

3. Parent Chain Traversal: The recursive value() search up the parent chain could be expensive for deep hierarchies.

🔒 Security Concerns

1. Process Termination: The ability to terminate the entire process from any context could be abused if context instances are passed to untrusted code.

2. Type Safety: The Any type usage is properly contained but still carries inherent risks of type confusion.

📋 Testing & Integration

1. Missing Tests:

   • No tests for throw_irrecoverable() behavior (understandable due to process exit)
   • No tests for deep context hierarchies
   • No concurrent access tests

2. Integration with BaseNode: The integration in BaseNode looks good and follows the expected patterns.

🚀 Minor Improvements

1. Dependency Addition: Good addition of tokio and tokio-util dependencies.

2. Module Structure: Clean module organization with examples.

3. Error Context: Good use of anyhow for error handling throughout.

📝 Recommendations

1. Critical: Refactor to follow repository design patterns (internal thread safety + shallow cloning)
2. High: Address memory leak potential in parent-child relationships
3. High: Consider alternatives to std::process::exit(1)
4. Medium: Replace unwrap() with proper error handling
5. Low: Add more comprehensive tests for edge cases

🎯 Overall Assessment

This is a well-designed API that successfully brings Go-like context patterns to Rust. However, it has critical architectural issues that violate the repository's established design patterns. The implementation needs refactoring to align with the codebase's thread safety and cloning conventions before it can be merged.

Recommendation: Requires changes before merge, primarily to align with repository design patterns.

---

[staheri14]

Closing this on favour of a more simplified version in #65