feat(gas): add batch listen #3478

MasterPtato · 2025-11-18T02:30:09Z

No description provided.

vercel · 2025-11-18T02:30:16Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
rivetkit-serverless	Ready	Preview	Comment	Dec 2, 2025 7:11pm

3 Skipped Deployments

Project	Deployment	Preview	Updated (UTC)
rivet-cloud	Ignored		Dec 2, 2025 7:11pm
rivet-inspector	Ignored	Preview	Dec 2, 2025 7:11pm
rivet-site	Ignored	Preview	Dec 2, 2025 7:11pm

MasterPtato · 2025-11-18T02:30:26Z

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

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

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.

claude · 2025-11-18T02:32:48Z

Pull Request Review: Batch Listen Feature

This PR introduces batch signal listening functionality to the workflow engine, allowing workflows to receive multiple signals in a single operation. This is a significant enhancement that can improve throughput and reduce overhead when dealing with multiple signals.

Summary

Changes: 1,215 additions, 732 deletions across 16 files
Core Feature: Transforms single-signal listening to batch-based signal processing

Positive Aspects

Architecture & Design

Well-structured approach: The batch listening is implemented at multiple layers (trait, context, database) with proper abstraction
Backwards compatibility: The single listen() method is preserved and now delegates to listen_n() with limit=1, maintaining existing API contracts
Consistent pattern: Both listen_with_timeout and listen_until variants support batch operations

Code Quality

Type safety: The Listen trait now returns Vec<Self>, enforcing that all signal types support batch operations
History tracking: New SignalsEvent properly tracks multiple signals with coordinated signal_ids, names, and bodies vectors
Error handling: Maintains existing error patterns while adapting to batch scenarios

Critical Issues

1. Breaking Change: Macro-generated `Listen` Implementation

Location: engine/packages/gasoline-macros/src/lib.rs:375-381

The #[signal] macro now generates a Listen implementation that returns Vec<Self>:

async fn listen(ctx: &mut gas::prelude::ListenCtx, limit: usize) -> gas::prelude::WorkflowResult<Vec<Self>> {
    ctx
        .listen_any(&[<Self as gas::signal::Signal>::NAME], limit)
        .await?
        .into_iter()
        .map(|signal| Self::parse(&signal.signal_name, &signal.body))
        .collect()
}

Issue: For a single signal type, this will return a Vec containing at most limit signals of the same type. However, if only 1 signal is available, it returns a 1-element vec. The consumer must now handle this differently.

Impact: All workflows using ctx.listen::<SomeSignal>() will need updates, or you need migration logic.

Recommendation: Document this breaking change clearly and consider:

Providing a migration guide for existing workflows
Adding deprecation warnings if keeping old single-signal methods temporarily

2. Inconsistent Limit Behavior

Location: engine/packages/gasoline/src/ctx/listen.rs:40-49

pub async fn listen_any(
    &mut self,
    signal_names: &[&'static str],
    limit: usize,
) -> WorkflowResult<Vec<SignalData>> {
    // ...
    if signals.is_empty() {
        return Err(WorkflowError::NoSignalFound(Box::from(signal_names)));
    }

Issue: The function fetches up to limit signals across ALL signal names, but the limit is applied per signal name in the database layer (line 1953 in db/kv/mod.rs). This creates ambiguity:

Does limit=5 mean 5 total signals, or 5 per signal type?
The current implementation fetches up to limit per signal type, then flattens them

Example Problem:

// Listening for 2 signal types with limit=5
// Could return up to 10 signals (5 per type)!
ctx.listen_any(&["signal_a", "signal_b"], 5).await?

Recommendation:

Clarify the limit semantics in documentation
Consider renaming to limit_per_type or enforcing a global limit by taking only the first N after flattening

3. Missing Signal ID Tracking

Location: engine/packages/gasoline/src/history/event.rs:210-214

pub struct SignalsEvent {
    pub names: Vec<String>,
    pub bodies: Vec<Box<serde_json::value::RawValue>>,
}

Issue: The public SignalsEvent struct in the event history doesn't include signal_ids, but the debug version does (line 1535 in db/kv/debug.rs). This makes it impossible to:

Correlate signals to their original IDs in user code
Debug which specific signals were processed
Implement proper idempotency checks

Recommendation: Add pub signal_ids: Vec<Id> to the public SignalsEvent struct.

4. Data Consistency Risk in History

Location: engine/packages/gasoline/src/db/kv/debug.rs:1542-1577

The TryFrom<WorkflowHistoryEventBuilder> for SignalsEvent checks that vectors are non-empty but doesn't validate they have the same length:

Ok(SignalsEvent {
    signal_ids: if value.indexed_signal_ids.is_empty() {
        return Err(WorkflowError::MissingEventData("signal_id"));
    } else {
        value.indexed_signal_ids
    },
    names: if value.indexed_names.is_empty() {
        return Err(WorkflowError::MissingEventData("name"));
    } else {
        value.indexed_names
    },
    bodies: if value.indexed_input_chunks.is_empty() {
        // ...
    }
})

Issue: If the vectors have different lengths (due to corruption or partial writes), this could cause panics or incorrect signal matching.

Recommendation: Add validation:

ensure!(
    value.indexed_signal_ids.len() == value.indexed_names.len() 
        && value.indexed_names.len() == value.indexed_input_chunks.len(),
    "signal event data vectors must have equal length"
);

Moderate Issues

5. Error Context Loss

Location: engine/packages/gasoline/src/ctx/workflow.rs:707

pub async fn listen<T: Listen>(&mut self) -> Result<T> {
    let signals = self.listen_n::<T>(1).in_current_span().await?;
    signals
        .into_iter()
        .next()
        .context("must return at least 1 signal")
}

Issue: The error message "must return at least 1 signal" is generic and doesn't indicate which signal type was expected. This makes debugging harder.

Recommendation:

.with_context(|| format!("must return at least 1 signal for type {}", std::any::type_name::<T>()))

6. Metrics Removed Without Explanation

Location: engine/packages/gasoline/src/ctx/listen.rs (removed lines 28-42)

The SIGNAL_PULL_DURATION metric was removed, but it provided valuable insight into database performance.

Recommendation: Either restore the metric with batch-aware labels, or document why it was removed in the commit message.

7. Unused Loop Location Parameter

Location: engine/packages/gasoline/src/db/kv/mod.rs:1922

_loop_location: Option<&Location>,

Issue: The parameter is prefixed with _, indicating it's unused. If it's not needed, remove it from the trait definition.

Recommendation: Clean up unused parameters or document why they're reserved for future use.

Minor Issues & Suggestions

8. Display Implementation Inefficiency

Location: engine/packages/gasoline/src/history/event.rs:77-83

EventData::Signals(signals) => {
    let mut unique_names = signals.names.clone();
    unique_names.sort();
    unique_names.dedup();
    write!(f, "signals {:?}", unique_names.join(", "))
}

Suggestion: This allocates and clones for every display call. Consider using a HashSet or caching if displayed frequently.

9. Naming: `last_attempt` vs `last_try`

Location: engine/packages/gasoline/src/ctx/listen.rs:17

The parameter was renamed from last_try to last_attempt. While more professional, ensure all documentation references are updated.

10. Test Coverage

Observation: No new test files were added for the batch listening functionality.

Recommendation: Add tests for:

Batch signal reception with multiple signals
Limit behavior with various signal counts
History replay with SignalsEvent
Edge cases (empty results, limit=0, etc.)

Security Considerations

11. Resource Exhaustion Risk

Location: Database signal fetching logic

Issue: The limit parameter is user-controlled (via listen_n()). If a workflow requests limit=1000000, it could:

Consume excessive memory
Cause database performance issues
Enable DoS attacks

Recommendation:

Add a maximum limit constant (e.g., MAX_BATCH_SIZE = 100)
Document the reasonable limit range
Consider adding metrics for large batch requests

Performance Considerations

Positive

Batch operations reduce database round-trips
Parallel signal stream fetching (line 1941-1972 in db/kv/mod.rs) is well-designed

Potential Concerns

Multiple signal types with large limits could cause memory spikes
No pagination for very large signal backlogs

Documentation & Style

Per CLAUDE.md:

✅ Uses structured logging correctly (e.g., tracing::info!(?x, "foo"))
✅ Lowercase log messages
✅ Proper error handling with anyhow::Result
✅ Follows timestamp naming (create_ts)
❌ Could use more inline documentation for complex functions like pull_next_signals

Recommendations

High Priority

Clarify limit semantics - Document whether limit is per-signal-type or total
Validate vector lengths in SignalsEvent deserialization
Add signal_ids to public API for better debugging and idempotency
Add integration tests for batch signal handling
Document breaking changes for workflows using the old API

Medium Priority

Restore or replace removed metrics
Add maximum limit validation to prevent resource exhaustion
Improve error messages with signal type information
Clean up unused parameters

Low Priority

Optimize Display implementation
Add inline documentation for complex database operations

Conclusion

This is a well-architected feature that adds valuable batch processing capabilities to the workflow engine. The implementation is mostly solid with good separation of concerns and proper abstraction layers.

However, there are critical issues around limit semantics and data consistency that should be addressed before merging. The breaking API changes also need clear documentation and migration guidance.

Overall Assessment: Approve with requested changes

Recommendation: Address critical issues #1-4, then merge. Other issues can be follow-up PRs.

🤖 Generated with Claude Code

claude · 2025-11-18T23:53:42Z

PR Review: feat(gas): add batch listen

Summary

This PR introduces batch signal listening to the Gasoline workflow engine, allowing workflows to listen for and process multiple signals at once instead of one at a time. This is a significant architectural improvement that can reduce latency and improve throughput for workflows that process multiple signals.

Code Quality & Best Practices

Strengths:

✅ Good use of structured logging with tracing (e.g., tracing::debug!(?x, "foo") pattern)
✅ Proper error handling with custom error types
✅ Backward compatibility: Old EventType::Signal is properly converted to the new EventType::Signals format
✅ Consistent naming conventions (snake_case, past tense for timestamps)
✅ Good separation of concerns between database layer and workflow context

Issues & Suggestions:

Missing signal_id in SignalsEvent (engine/packages/gasoline/src/history/event.rs:210-214)
```
pub struct SignalsEvent {
    pub names: Vec<String>,
    pub bodies: Vec<Box<serde_json::value::RawValue>>,
}
```
The SignalsEvent struct is missing signal_ids: Vec<Id> field that appears to be used in the debug implementation and history keys. This could cause issues when replaying events or debugging.
Potential ordering issue (engine/packages/gasoline/src/db/kv/mod.rs:2059-2063)
```
// Sort by ts
signals.sort_by_key(|key| key.create_ts);

// Apply limit
Ok(signals.into_iter().take(limit).collect())
```
The limit is applied AFTER sorting, but signals are fetched with a limit from each signal name subspace. If you have 100 signals of type A and 1 of type B, and request limit=10, you might get all 10 from A and miss the one from B even if B is older. Consider sorting BEFORE fetching or using a priority queue approach.
Deprecated metric removed (engine/packages/gasoline/src/metrics.rs)
The SIGNAL_PULL_DURATION metric was removed but might still be valuable for performance monitoring. Consider keeping it and recording the total duration for pulling all signals in a batch.
API Breaking Change
The Listen trait now requires returning Vec<Self> instead of Self, and CustomListener trait was removed. This is a breaking change that should be:
- Documented in a changelog/migration guide
- Validated that all existing implementations are updated

Potential Bugs

Empty signals vec guarantee (engine/packages/gasoline/src/ctx/workflow.rs:703-710)
```
pub async fn listen<T: Listen>(&mut self) -> Result<T> {
    let signals = self.listen_n::<T>(1).in_current_span().await?;

    signals
        .into_iter()
        .next()
        .context("must return at least 1 signal")
}
```
The code assumes listen_n will never return an empty vec, but this isn't enforced by the type system. If listen_n returns empty, this will panic with "must return at least 1 signal". Consider adding an assertion in listen_n or handling this more gracefully.
Race condition in listen_n_until (engine/packages/gasoline/src/ctx/workflow.rs:1154-1182)
After a sleep is interrupted, the code checks for signals in history. However, if the history check finds HistoryResult::New instead of signals, it continues to listen for new signals. There's a potential edge case where signals could be missed between the sleep interruption and the history check.
Buffer size assumption (engine/packages/gasoline/src/db/kv/mod.rs:2055)
```
.buffer_unordered(1024)
```
Hard-coded buffer of 1024 could be problematic if limit is much smaller or larger. Consider using buffer_unordered(limit.max(16).min(1024)) for better performance.

Performance Considerations

Improvements:

✅ Batching reduces database round trips
✅ Using buffer_unordered for concurrent signal processing
✅ Sorting and limiting happen after fetching, avoiding multiple DB queries

Concerns:

Flattening signal streams (engine/packages/gasoline/src/db/kv/mod.rs:1941-1961)
```
futures_util::stream::iter(owned_filter.clone())
    .map(|signal_name| { /* ... */ })
    .flatten()
```
Using .flatten() on multiple range queries could lead to uneven fetching if one signal type has many more entries than others. The current approach fetches limit from EACH signal type, then sorts and takes limit total. This could fetch more data than needed.
Multiple indexed key writes (engine/packages/gasoline/src/db/kv/keys/history.rs)
The new implementation writes separate keys for:
- Each signal ID (IndexedSignalIdKey)
- Each signal name (IndexedNameKey)
- Each signal input chunk (IndexedInputChunkKey)
This could significantly increase write amplification. For N signals with M chunks each, you're writing ~N*(2+M) keys instead of N keys. Consider benchmarking the performance impact.

Security Concerns

No validation on limit parameter
The limit parameter in listen_n is not validated. A malicious or buggy caller could pass usize::MAX which could cause:
- Excessive memory allocation
- Database query timeouts
- DoS conditions
Recommend adding: let limit = limit.min(MAX_SIGNAL_BATCH_SIZE); with a reasonable constant.
Signal body size not validated
When combining chunks into signal bodies, there's no validation that the total size is reasonable. Large signal bodies could cause OOM issues.

Test Coverage

Missing:

❌ No test files modified in this PR
❌ No tests for listen_n functionality
❌ No tests for batch signal processing
❌ No tests for the new SignalsEvent type
❌ No tests for backward compatibility (old Signal → new Signals conversion)
❌ No tests for limit parameter edge cases (0, 1, large numbers)
❌ No tests for signal ordering when batching multiple signal types

Recommended Tests:

// Test batch listen with multiple signals
#[tokio::test]
async fn test_listen_n_multiple_signals() {
    // Send 5 signals, listen for 3, verify we get the oldest 3
}

// Test listen_n with limit larger than available signals
#[tokio::test]  
async fn test_listen_n_limit_exceeds_available() {
    // Send 2 signals, listen for 5, verify we get 2
}

// Test backward compatibility
#[tokio::test]
async fn test_old_signal_event_conversion() {
    // Load workflow with old Signal events, verify they convert to Signals
}

// Test signal ordering across types
#[tokio::test]
async fn test_listen_n_mixed_signal_types() {
    // Send signals of type A and B interleaved by time
    // Verify they're received in chronological order
}

Architecture & Design

Positive:

The batch approach is a good architectural decision for high-throughput scenarios
Clean separation between listen() (single) and listen_n() (batch) maintains backward compatibility at the API level
The history cursor changes properly support the new batch model

Questions:

Why was CustomListener removed? Were there no use cases for it, or does batch listening subsume its functionality?
Should there be a way to listen for "all available signals up to N" vs "exactly N signals or error"? The current implementation errors if fewer signals are available.

Summary & Recommendations

This is a well-structured PR that adds valuable batching capabilities to the workflow engine. However, it needs:

Must Fix:

Add signal_ids field to SignalsEvent struct
Add validation for limit parameter to prevent DoS
Fix the ordering issue in signal fetching (fetch limit total, not limit per type)
Add comprehensive test coverage

Should Fix:

Consider keeping SIGNAL_PULL_DURATION metric
Document the breaking changes to Listen trait
Add buffer size tuning based on limit
Add signal body size validation

Nice to Have:

Add migration guide for users of old CustomListener trait
Benchmark write amplification impact
Consider exposing batch size as a configuration parameter

The core implementation is solid, but the missing tests and potential edge cases around limits and ordering need to be addressed before merging.

Generated by Claude Code

claude · 2025-11-18T23:54:19Z

Code Review

Summary

This PR adds batch signal listening capabilities to the Gasoline workflow engine, allowing workflows to receive multiple signals in a single operation instead of one at a time. This is a significant architectural improvement that should improve efficiency when dealing with high-volume signal processing.

Architecture Changes

✅ Well-designed changes:

Signal History Consolidation: The new SignalsEvent replaces individual signal events in history, batching multiple signals into a single event. This is more efficient and cleaner than the previous approach.
Indexed Storage: The implementation uses indexed keys (IndexedSignalIdKey, IndexedNameKey, IndexedInputKey) to store multiple signals within a single history event, which is a clean database schema design.
Backward Compatibility: The PR properly deprecates the old Signal event type (EventType::Signal = 1) while introducing Signals (EventType::Signals = 10), maintaining compatibility.

Code Quality

✅ Strengths:

Consistent API Design: The new listen_n() and listen_n_with_timeout() methods follow the existing naming conventions and patterns.
Proper Error Handling: Maintains proper error handling throughout, including history divergence checks in compare_signals().
Metrics & Observability: Maintains logging and metrics (signal receive lag tracking, etc.).
Macro Updates: The #[signal] macro and join_signal\! macro properly updated to return Vec<Self> instead of single values.

Potential Issues

⚠️ Critical Issues:

1. EventType Mismatch in insert::signals_event (packages/gasoline/src/db/kv/keys/history.rs:1645)

pub fn signals_event(/* ... */) -> Result<()> {
    common(
        subspace,
        tx,
        workflow_id,
        location,
        EventType::Signal,  // ❌ Should be EventType::Signals
        version,
        create_ts,
    )
}

This appears to use EventType::Signal (the deprecated type) instead of EventType::Signals. This could cause history corruption or replay issues.

Recommended fix:

EventType::Signals,  // Use the new batch signal type

2. Missing Validation on Signal Count

The SignalsEvent struct (packages/gasoline/src/history/event.rs:211-214) stores names and bodies as separate vectors but does not enforce that they have the same length:

pub struct SignalsEvent {
    pub names: Vec<String>,
    pub bodies: Vec<Box<serde_json::value::RawValue>>,
}

In the database deserialization code (packages/gasoline/src/db/kv/debug.rs:1545), there is a zip operation that could silently drop data if lengths do not match. Consider adding validation:

pub struct SignalsEvent {
    pub names: Vec<String>,
    pub bodies: Vec<Box<serde_json::value::RawValue>>,
}

impl SignalsEvent {
    pub fn new(names: Vec<String>, bodies: Vec<Box<serde_json::value::RawValue>>) -> Result<Self> {
        ensure\!(names.len() == bodies.len(), "signals names and bodies must have same length");
        ensure\!(\!names.is_empty(), "signals event must contain at least one signal");
        Ok(Self { names, bodies })
    }
}

⚠️ Medium Priority Issues:

3. Removed CustomListener Without Clear Migration Path

The PR removes the CustomListener trait and custom_listener() method entirely:

Removed from packages/gasoline/src/ctx/versioned_workflow.rs (lines 128-136)
Not mentioned in PR description

If any users were using CustomListener, this is a breaking change. Consider:

Adding a deprecation note/migration guide
Ensuring no internal code was using this functionality
Documenting this breaking change

4. Potential Performance Issue: Sort After Parallel Fetch

In pull_next_signals (packages/gasoline/src/db/kv/mod.rs:2059-2063):

.buffer_unordered(1024)
.try_collect::<Vec<_>>()
.await?;

// Sort by ts
signals.sort_by_key(|key| key.create_ts);

// Apply limit
Ok(signals.into_iter().take(limit).collect())

The code fetches up to limit signals from each signal name stream in parallel, then sorts all results by timestamp, then takes limit items. This means:

If listening for N signal types with limit=10, you might fetch 10*N signals then discard all but 10
The buffer_unordered(1024) suggests very high parallelism but the final limit might waste most of that work

Recommendation: Consider limiting the total fetch earlier or documenting the tradeoff.

Performance Considerations

✅ Good:

Batching signals reduces database round-trips
Parallel signal body fetching with buffer_unordered(1024)
Maintains existing polling/subscription mechanisms

⚠️ Concern:

The listen_n semantics could lead to workflows requesting large batches unnecessarily if not used carefully
No apparent safeguard against very large batch sizes (could the limit parameter be validated against a max?)

Security

✅ No obvious security concerns

Maintains existing workflow isolation
No new injection vectors
Proper deserialization error handling

Test Coverage

❌ Critical Gap: No Tests for Batch Listening

The PR adds significant new functionality but I could not find any tests for:

listen_n() functionality
listen_n_with_timeout()
listen_n_until()
Multiple signals in a single SignalsEvent
History replay with batched signals

Recommendation: Add tests covering:

Basic listen_n(limit=5) with multiple signals arriving
Behavior when fewer than limit signals arrive
Correct ordering of batched signals by timestamp
History replay with SignalsEvent containing multiple signals
Timeout behavior with listen_n_with_timeout

Example test case:

#[workflow_test]
async fn test_batch_signal_listen() {
    // Publish 5 signals
    // ctx.listen_n::<MySignal>(10) should return all 5
    // Verify order matches timestamps
}

Additional Observations

Good: The listen() method is now implemented as listen_n(1) which is a clean refactor that ensures consistency.
Good: Updated print/debug utilities to handle the new SignalsEvent type.
Question: The PR description is empty. Consider adding:
- Why batch listening is needed
- Performance improvement metrics (if available)
- Breaking changes documentation
- Migration guide for CustomListener users

Recommendations

Must Fix:

Fix EventType::Signal → EventType::Signals in insert::signals_event
Add validation to SignalsEvent to ensure names.len() == bodies.len()
Add comprehensive test coverage for batch listening

Should Consider:

Add PR description explaining the changes and migration path
Document the breaking change (removal of CustomListener)
Review the fetch-all-then-limit performance pattern in pull_next_signals
Add max batch size validation to prevent abuse

Nice to Have:

Add examples of using listen_n in documentation
Performance benchmarks showing improvement over sequential signal listening

Verdict

This is a well-architected feature that makes logical sense for the workflow engine. However, it has a critical bug (wrong EventType) and lacks test coverage. Once these issues are addressed, this will be a solid improvement.

Status: Needs Changes 🔴

The EventType bug could cause production issues with history replay, and the lack of tests makes it risky to merge. Please address the critical issues before merging.

claude · 2025-11-19T22:11:25Z

PR Review: Add Batch Listen

Overview

This PR introduces batch signal listening capabilities to the Gasoline workflow engine, allowing workflows to receive multiple signals in a single operation.

Strengths

1. Well-Structured API Changes

Listen trait now returns Vec - clean abstraction
Backward compatibility via listen() calling listen_n(1)
Clear batch semantics with listen_n() and listen_n_with_timeout()

2. Comprehensive Database Implementation

Well-designed indexed signal storage keys (IndexedSignalIdKey, IndexedNameKey, IndexedInputKey)
Proper chunked keys for large signal bodies
SignalsEvent type properly supports multiple signals

3. Good Error Handling

Empty vecs trigger NoSignalFound appropriately (ctx/listen.rs:68)
History divergence checks validate signal events (history/cursor.rs:561-590)

4. Logging and Observability

Structured logging with signal counts, IDs, names
Per-signal metrics for receive lag
Enhanced debug printing (engine/src/util/wf/mod.rs:470-497)

Concerns

1. Breaking API Change ⚠️
Listen trait signature changed. Any custom implementations will break.

Recommendations:

Document as breaking change in PR description
Search for manual Listen implementations
Add migration notes

2. Potential Index Corruption 🔴
db/kv/debug.rs:897-940 rebuilds events from indexed keys with ordering checks. Partial transaction failures could cause corruption on replay.

Recommendations:

Add integration tests for partial write failures
Consider repair/recovery logic
Document invariants

3. Empty Vec Semantics ⚠️
Inconsistent behavior:

listen_n_with_timeout() → empty vec valid (timeout)
listen_n() → empty vec errors

Recommendation: Document when empty vecs are expected vs errors

4. Test Coverage Gaps 🟡
Missing tests for:

Batch listening with n > 1
Mixed signal types in batches
Replay with batch signals
Edge cases (more requested than available)

5. Performance 💭
Sequential processing of signals for metrics/logging could add latency for large batches.

6. Magic Number 🟡
recv_lag > 3.0 check needs constant and explanation (ctx/listen.rs:82)

7. Removed EventData::Empty
Was Empty unused or does this affect backfilling?

Security & Safety

✅ No security concerns
✅ Proper transactions and conflict ranges
✅ Signal ack timestamps correct

Conclusion

Well-implemented feature! Main concerns:

Breaking change documentation needed
Index corruption risk - needs testing
Batch-specific test coverage needed

Recommendation: Add breaking change docs and batch tests before merging.

Overall: Approve with comments 🎉

claude · 2025-11-19T22:12:40Z

PR Review: feat(gas): add batch listen

This PR introduces batch signal listening to the Gasoline workflow engine, allowing workflows to receive multiple signals in a single operation. This is a significant performance optimization.

Architecture Changes

Core API Changes:

Listen trait now returns Vec and accepts a limit parameter
WorkflowCtx::listen() now internally calls listen_n(1)
New methods: listen_n(), listen_n_with_timeout(), and listen_n_until()
Removed CustomListener trait

Database Layer:

New pull_next_signals() method with parallel signal fetching
Signals sorted by timestamp and limited to requested count
New EventType::Signals replaces individual signal events

History Management:

New SignalsEvent with indexed data structures
New history keys for indexed signals
New cursor method: compare_signals()

Code Quality - Strengths

Excellent use of parallel processing with buffer_unordered(1024)
Proper transaction boundaries with conflict ranges
Maintains backward compatibility
Comprehensive tracing and metrics
Proper error propagation

Areas for Improvement

Transaction Size (db/kv/mod.rs:1988-2057): Large batches could create very large transactions. Consider transaction size limits.
Sorting After Limit (db/kv/mod.rs:2059-2063): Fetches limit signals from each stream, then sorts and limits again. Could fetch more than needed. Add clarifying comment or adjust logic.
Error Context (ctx/workflow.rs:707-708): Use expect() instead of context() since empty result indicates a bug.

Potential Bugs

Index Ordering (db/kv/debug.rs:901-927): Assumes indices arrive in order. Use HashMap for robustness.
Empty Signals Vec (ctx/workflow.rs:1165-1177): When replaying with SleepState::Interrupted, code assumes non-empty vec. Add validation.

Performance

Positive:

Parallel signal fetching is excellent
Single transaction reduces round trips

Concerns:

No bounds on signal body sizes (memory pressure)
Metrics recorded in loop for each signal (could be slow for large batches)

Security

No significant issues. Proper conflict ranges prevent races, signal ack prevents double-delivery.

Test Coverage

Missing: No test files in PR. Critical tests needed:

listen_n() with limit > available signals
Signal ordering with mixed timestamps
History replay with batched signals
listen_n_until() timeout scenarios
Transaction conflicts during batch pull

Best Practices

Follows CLAUDE.md conventions well (structured logging, lowercase messages, anyhow::Result, proper timestamps).

Minor: Some functions exceed reasonable length (180 lines). Consider extracting helpers.

Final Recommendation

APPROVE with minor recommendations

Well-architected change with meaningful performance improvements. Code quality is high and follows conventions.

Before merging:

Add integration tests
Address empty signals vector edge case
Add clarifying comments for per-stream limit logic
Consider transaction size limits

Nice work! The parallel signal fetching is particularly well done.

claude · 2025-11-26T02:22:52Z

PR Review: feat(gas): add batch listen

Summary

This PR introduces batch signal listening functionality to the Gasoline workflow engine, allowing workflows to receive multiple signals at once instead of one at a time. This is a significant architectural change that affects the core signal handling mechanisms.

Code Quality & Architecture

Strengths

Well-structured change: The refactoring systematically updates the signal handling from single to batch operations across all layers (API, database, history, cursor).
Backward compatibility preserved: The listen() method still works as before by delegating to listen_n(1) and extracting the first signal, ensuring existing code continues to work.
Comprehensive implementation: The change properly propagates through all layers - from the trait definition to the database implementation to the history cursor.
Good error handling: The code properly handles edge cases like empty signal lists and maintains error context through the stack.

Areas of Concern

1. Breaking API Change in Listen trait (High Severity)

The Listen::listen signature changed from async fn listen(ctx: &mut ListenCtx) -> WorkflowResult to async fn listen(ctx: &mut ListenCtx, limit: usize) -> WorkflowResult<Vec>

Issue: This is a breaking change for any custom implementations of the Listen trait outside the codebase. The #[signal] macro was updated to generate the new signature, but any hand-written implementations will break.

Recommendation: Consider deprecation path or document this as a breaking change in release notes.

2. Inconsistent Error Handling (Medium Severity)

In workflow.rs:703-709, the .context() error message for an empty vec case could be more helpful for debugging.

Recommendation: Use a more specific error or add debug assertions.

3. Removed CustomListener Trait (Medium Severity)

The PR removes the CustomListener trait and custom_listener method without explanation.

Files affected:

gasoline/src/listen.rs (trait removed)
gasoline/src/ctx/versioned_workflow.rs:125-135 (method removed)

Recommendation: Document why this was removed.

4. History Storage Complexity (Medium Severity)

The new Signals event type stores multiple signals with indexed storage. The deserialization logic in db/kv/debug.rs has checks that assume indexes arrive in order (0, 1, 2...).

Recommendation: Add documentation about ordering guarantees.

5. Missing Test Coverage (High Severity)

No new tests were added for the batch listen functionality.

Missing test scenarios:

Multiple signals received in one batch
Batch size limits (what happens with limit parameter?)
Order preservation when multiple signals arrive
Interaction with listen_with_timeout and batches
The new listen_n and listen_n_with_timeout methods

Recommendation: Add comprehensive tests in tests/workflows/signal_test.rs.

Performance Considerations

Positive Impact

Reduced polling overhead: Batch receiving reduces database round-trips
Better throughput: Workflows can process multiple signals in one wake cycle

Potential Concerns

Memory usage: Batching could increase memory pressure if many large signals are fetched at once. Consider documenting recommended limit values.

Migration & Compatibility

Breaking Changes

Listen trait signature changed (affects custom implementations)
CustomListener trait removed
Database schema evolved (new EventType::Signals = 10)

Questions

Database migration: Does the new EventType::Signals require a migration for existing workflows?
Backward compatibility: Can old workflow history still be replayed?

Recommendations Summary

Must Fix (High Priority)

Add comprehensive test coverage for batch signal functionality
Document the breaking changes for Listen trait implementers
Clarify database migration requirements and backward compatibility

Should Fix (Medium Priority)

Improve error message in listen() method for empty vec case
Document removal of CustomListener trait
Add doc comments explaining the limit parameter and recommended values
Verify ordering guarantees in indexed signal storage

Conclusion

This is a solid architectural improvement that enables important workflow batching capabilities. The implementation is generally well-structured, but needs test coverage, documentation of breaking changes, and clarification on database migration/compatibility.

The code quality is good and follows the existing patterns in the codebase.

Overall Assessment: ⚠️ Approve with Changes Required

Please address the test coverage and breaking change documentation before merging.

claude · 2025-11-26T02:23:55Z

Code Review: feat(gas): add batch listen

Overview

This PR adds batch listening capability to the Gasoline workflow engine, allowing workflows to receive multiple signals in a single operation. This is a significant enhancement that improves efficiency for workflows that need to process multiple signals.

✅ Strengths

Well-designed API changes: The refactoring from single signal to batch signals is clean and maintains backward compatibility through the listen() method which now wraps listen_n(1).
Proper backward compatibility: The migration from deprecated EventType::Signal to EventType::Signals is handled gracefully in db/kv/mod.rs:2898-2910, converting old single-signal events to the new batch format.
Good data structures: The new SignalsEvent struct uses parallel vectors (names, bodies, signal_ids) which is a reasonable approach for storing batch data.
Comprehensive history tracking: The indexed keys (IndexedSignalIdKey, IndexedNameKey, IndexedInputKey) properly track multiple signals per event.

⚠️ Issues & Concerns

1. Critical: Potential data inconsistency in SignalsEvent

Location: engine/packages/gasoline/src/history/event.rs:211-214

The SignalsEvent struct has three parallel vectors but no invariant enforcement:

pub struct SignalsEvent {
    pub names: Vec<String>,
    pub bodies: Vec<Box<serde_json::value::RawValue>>,
}

Problem: There's a missing signal_ids field in the public struct definition, but the debug code references it (db/kv/debug.rs:1542). This suggests either:

Missing field in the struct definition
Or the vectors can become desynchronized (different lengths)

Recommendation:

Add signal_ids: Vec<Id> to the struct
Add validation ensuring all three vectors have the same length
Consider using a Vec<Signal> with a proper struct instead of parallel vectors to prevent desynchronization

2. Bug: Incorrect EventType in history insertion

Location: engine/packages/gasoline/src/db/kv/keys/history.rs:1645

pub fn signals_event(
    // ...
) -> Result<()> {
    common(
        subspace,
        tx,
        workflow_id,
        location,
        EventType::Signal,  // ⚠️ Should be EventType::Signals
        version,
        create_ts,
    )
}

Problem: Using deprecated EventType::Signal instead of EventType::Signals for new events. While backward compatibility handles reading old events, new events should use the correct type.

Impact: Could cause issues with event type matching and future migrations.

Fix: Change line 1645 to EventType::Signals

3. Performance: Unbounded buffer in signal processing

Location: engine/packages/gasoline/src/db/kv/mod.rs:2052

.buffer_unordered(1024)

Problem: Hard-coded buffer size of 1024 for parallel signal processing. If limit is less than 1024, this allocates unnecessary resources. If limit is greater, it could cause memory pressure.

Recommendation: Use buffer_unordered(limit.min(1024)) or make it configurable.

4. Logic issue: Sorting after limiting

Location: engine/packages/gasoline/src/db/kv/mod.rs:2056-2060

// Sort by ts
signals.sort_by_key(|key| key.create_ts);

// Apply limit
Ok(signals.into_iter().take(limit).collect())

Problem: The limit is already applied at line 1950 when fetching from the database:

limit: Some(limit),

Then signals are processed with buffer_unordered which can reorder them, sorted by timestamp, and then limited again. This double-limiting with sorting in between might not give you the oldest limit signals across all signal types.

Scenario: If you have 100 signals each of types A and B, and request limit=50, you might get 50 from A and 50 from B (total 100), sort them, then take the first 50 by timestamp. This might not be the intended behavior.

Recommendation: Clarify the intended semantics and potentially fetch with a higher limit, then sort and take.

5. Missing test coverage

Location: No new test files for batch functionality

Problem: The existing test file tests/workflows/signal_test.rs only tests single signal reception. There are no tests for:

Multiple signals received in one batch
Mixed signal types
Batch limits
listen_n_with_timeout functionality
Edge cases (empty results, limit=0, etc.)

Recommendation: Add comprehensive tests for the new batch listening features.

📝 Minor Issues

6. Inconsistent naming

last_try → last_attempt is a good rename, but check if this is used consistently everywhere

7. Documentation

The new listen_n and listen_n_with_timeout methods lack doc comments
The limit parameter in the Listen trait should document behavior when limit=0

8. Removed CustomListener without deprecation notice

Location: engine/packages/gasoline/src/ctx/versioned_workflow.rs:125-136 (deleted)

The custom_listener method was removed entirely. If this was part of the public API, consider:

Adding a deprecation notice in the changelog
Checking if any code depends on this

🔒 Security

No security concerns identified. The changes maintain proper transaction boundaries and don't introduce obvious vulnerabilities.

🎯 Recommendations Summary

Must Fix:

Fix EventType::Signal → EventType::Signals in history insertion (bug)
Add signal_ids field to SignalsEvent or clarify the design
Add validation for parallel vector consistency

Should Fix:

Clarify and fix the sorting/limiting logic for signals
Add comprehensive test coverage
Adjust buffer_unordered size based on limit
Add documentation for new public methods

Nice to Have:

Consider refactoring parallel vectors to a proper struct
Document breaking changes if CustomListener was public API

Overall Assessment

This is a solid feature addition with good architectural decisions. The main concerns are around data consistency guarantees and the correctness of the event type in new insertions. Once the critical issues are addressed, this will be a valuable enhancement to the workflow engine.

The backward compatibility handling is particularly well done, and the overall code quality follows the project conventions.

claude · 2025-11-27T01:00:08Z

Pull Request Review: feat(gas): add batch listen

This PR adds batch signal listening capabilities to the Gasoline workflow engine, allowing workflows to efficiently receive multiple signals at once. Overall, this is a well-structured enhancement with good attention to detail. Below are my findings:

Strengths

1. API Design

Clean, composable API that maintains backward compatibility
listen() now calls listen_n(1) internally - elegant delegation pattern
Clear naming conventions: listen_n, listen_n_with_timeout, listen_n_until
The limit parameter allows flexible batch sizes

2. Database Implementation

The pull_next_signals implementation (lines 1912-2092 in db/kv/mod.rs) is well done:

Properly uses parallel streams to fetch from multiple signal types concurrently
Correctly sorts signals by timestamp after fetching (line 2057)
Applies limit correctly at the end (line 2060)
Maintains proper conflict ranges and serializable isolation

3. History Event Handling

New Signals event type properly replaces the Empty placeholder pattern
Indexed storage (IndexedSignalIdKey, IndexedNameKey, IndexedInputChunkKey) allows efficient batch serialization
Debug output correctly handles multiple signals (engine/packages/engine/src/util/wf/mod.rs:467-497)

4. Macro Updates

The #[signal] macro properly adapts to the new trait signature, requiring all implementations to return Vec.

Issues Found

1. Critical: Potential Signal Loss in pull_next_signals

Location: engine/packages/gasoline/src/db/kv/mod.rs:1938-1970

You are applying the limit twice:

First when fetching from each signal stream (line 1950)
Second after sorting and combining all signals (line 2060)

Problem: If you have 3 signal types and limit=5:

You fetch up to 5 signals from each type (15 total)
Then you take only the first 5 after sorting

This means signals with later timestamps might never be considered, even if they arrived before signals in other streams.

Fix: Remove the limit from the RangeOption and only apply it after sorting, or use a larger multiplier like limit * owned_filter.len().

2. Error Handling: Empty Vec Validation

Location: engine/packages/gasoline/src/ctx/workflow.rs:703-709

The error message does not include context about which signal type failed. Consider adding the type name to the error context.

3. Missing Documentation

Location: Multiple files

The new public APIs lack doc comments:

listen_n (line 713)
listen_n_with_timeout (line 1091)
listen_n_until (line 1123)

Add comprehensive doc comments explaining the limit parameter, ordering guarantees, return value guarantees, and example usage.

4. Inconsistent Naming in ListenCtx

Location: engine/packages/gasoline/src/ctx/listen.rs:17,27

You renamed last_try to last_attempt which is good, but the field documentation does not explain what "last attempt" means in the context of the retry logic.

Potential Issues

1. Performance: Unbounded Buffering

Location: engine/packages/gasoline/src/db/kv/mod.rs:2052

You are processing up to 1024 signals concurrently with .buffer_unordered(1024). For workflows with very large batches, this could cause memory pressure. Is there a realistic upper bound on the limit parameter?

2. Removed custom_listener API

Location: engine/packages/gasoline/src/ctx/versioned_workflow.rs:128-136

You completely removed the custom_listener API. Was this intentional? Were there any external uses of the CustomListener trait? This is a breaking API change.

3. Metric Granularity Lost

Location: engine/packages/gasoline/src/ctx/listen.rs:47-82

Previously, SIGNAL_PULL_DURATION was recorded per signal. Now it is not recorded at all. This removes observability into signal fetch performance. Consider adding a metric for batch size and batch pull duration.

Minor Issues

1. Type Annotation Redundancy

Location: engine/packages/gasoline/src/db/kv/mod.rs:1923

Could use explicit Vec instead of Vec<_> for clarity.

2. Unused Parameter

Location: engine/packages/gasoline/src/db/kv/mod.rs:1915,1919

Parameters prefixed with _ (_workflow_name, _loop_location) suggest they are intentionally unused. Verify these are not needed for the new batch logic.

3. Comment Formatting

Location: Multiple history key implementations

The new IndexedSignalIdKey, IndexedNameKey, etc. lack module-level documentation explaining the indexing scheme and why it is necessary.

Testing Recommendations

Add explicit batch listen tests:
- Test receiving multiple signals of the same type
- Test receiving signals of different types in the correct timestamp order
- Test limit enforcement
- Test that signals are properly acked and removed from pending
Edge cases:
- What happens when limit=0?
- What happens when more signals arrive than the limit?
- Test replay of batched signals from history
Performance tests:
- Verify memory usage with large batches
- Confirm timestamp ordering is maintained correctly

Code Quality Summary

Architecture: Good - Clean separation of concerns
Error Handling: Needs improvement - Error contexts could be more descriptive
Performance: Potential issue with double limiting
Testing: No new tests visible in PR
Documentation: Missing - Public APIs need doc comments
Security: Good - Proper conflict ranges and isolation levels

Recommendation

Request Changes - Address the critical signal loss issue in pull_next_signals before merging. The other issues are important but less critical.

Priority Fixes:

[Critical] Fix double limit application in pull_next_signals
[High] Add documentation for new public APIs
[Medium] Restore or explain removal of metrics and custom_listener
[Medium] Add tests for batch signal behavior

Great work on this feature! The core implementation is solid; it just needs these refinements before merging.

graphite-app · 2025-12-03T00:33:04Z

Merge activity

Dec 3, 12:33 AM UTC: NathanFlurry added this pull request to the Graphite merge queue.
Dec 3, 12:34 AM UTC: CI is running for this pull request on a draft pull request (#3577) due to your merge queue CI optimization settings.
Dec 3, 12:34 AM UTC: Merged by the Graphite merge queue via draft PR: #3577.

vercel bot temporarily deployed to Preview – rivetkit-serverless November 18, 2025 02:30 Inactive

This was referenced Nov 18, 2025

chore(gas): add test for history with holes #3477

Closed

fix(gas): fix batch listen, fix history for graceful signal send in workflows #3479

Closed

fix(pb): rewrite runner wf to handle batch signals #3480

Closed

MasterPtato force-pushed the 11-17-feat_gas_add_batch_listen branch from 25ece3d to 6ff29f6 Compare November 18, 2025 23:50

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 5c9b75f to 0471792 Compare November 18, 2025 23:50

This was referenced Nov 18, 2025

fix(gas): fix loop forgotten bug due to concurrency #3482

Closed

fix(pb): simplify runner wf #3483

Closed

vercel bot had a problem deploying to Preview – rivetkit-serverless November 18, 2025 23:53 Failure

MasterPtato force-pushed the 11-17-feat_gas_add_batch_listen branch from 6ff29f6 to dba469c Compare November 19, 2025 22:06

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 0471792 to f508120 Compare November 19, 2025 22:06

This was referenced Nov 19, 2025

chore(engine-runner): remove echo close on ToClientWebSocketClose #3487

Closed

chore(pegboard-gateway): remove TunnelAck #3488

Closed

fix(tunnel): implement ping pong #3491

Closed

NathanFlurry mentioned this pull request Nov 19, 2025

chore(pegboard-gateway): add new message id format & add deprecated tunnel ack #3492

Closed

vercel bot had a problem deploying to Preview – rivetkit-serverless November 19, 2025 22:07 Failure

MasterPtato force-pushed the 11-17-feat_gas_add_batch_listen branch from dba469c to 2a53bcf Compare November 20, 2025 03:07

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 7034a04 to 7d477c8 Compare November 26, 2025 02:16

MasterPtato force-pushed the 11-17-feat_gas_add_batch_listen branch from f9664d9 to a7d5ffd Compare November 26, 2025 02:16

vercel bot deployed to Preview – rivetkit-serverless November 26, 2025 02:18 View deployment

MasterPtato marked this pull request as ready for review November 26, 2025 23:36

MasterPtato requested a review from NathanFlurry November 26, 2025 23:36

This was referenced Nov 27, 2025

fix(runtime): evict runners on SIGTERM, add periodic shutdown progress messages #3550

Closed

chore(gh): make claude edit existing discussion comment for subsequent reviews #3551

Closed

MasterPtato force-pushed the 11-17-feat_gas_add_batch_listen branch from a7d5ffd to 41442d3 Compare November 27, 2025 00:52

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 7d477c8 to 21b69e8 Compare November 27, 2025 00:52

vercel bot deployed to Preview – rivetkit-serverless November 27, 2025 00:53 View deployment

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from 21b69e8 to cb268ef Compare November 27, 2025 00:58

MasterPtato force-pushed the 11-17-feat_gas_add_batch_listen branch from 41442d3 to 3f3140e Compare November 27, 2025 00:58

vercel bot deployed to Preview – rivetkit-serverless November 27, 2025 00:59 View deployment

feat(gas): add batch listen

d321009

MasterPtato force-pushed the 11-17-feat_gas_add_batch_listen branch from 3f3140e to d321009 Compare December 2, 2025 19:10

MasterPtato force-pushed the 11-17-chore_gas_add_test_for_history_with_holes branch from cb268ef to a9857fb Compare December 2, 2025 19:10

This was referenced Dec 2, 2025

feat(pegboard): move pegboard serverless to wf #3416

Closed

fix(serverless): general cleanup #3450

Closed

fix(serverless): drain runners after url/headers change #3571

Closed

vercel bot deployed to Preview – rivetkit-serverless December 2, 2025 19:11 View deployment

NathanFlurry approved these changes Dec 2, 2025

View reviewed changes

This was referenced Dec 2, 2025

fix(serverless): fix connection wf lifecycle #3573

Closed

fix(serverless): misc pr fixes #3576

Closed

graphite-app bot pushed a commit that referenced this pull request Dec 3, 2025

feat(gas): add batch listen (#3478)

72446c9

graphite-app bot closed this Dec 3, 2025

graphite-app bot deleted the 11-17-feat_gas_add_batch_listen branch December 3, 2025 00:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(gas): add batch listen #3478

feat(gas): add batch listen #3478

Uh oh!

MasterPtato commented Nov 18, 2025

Uh oh!

vercel bot commented Nov 18, 2025 •

edited

Loading

Uh oh!

MasterPtato commented Nov 18, 2025 •

edited

Loading

Uh oh!

claude bot commented Nov 18, 2025

Uh oh!

claude bot commented Nov 18, 2025

Uh oh!

claude bot commented Nov 18, 2025

Uh oh!

claude bot commented Nov 19, 2025

Uh oh!

claude bot commented Nov 19, 2025

Uh oh!

claude bot commented Nov 26, 2025

Uh oh!

claude bot commented Nov 26, 2025

Uh oh!

claude bot commented Nov 27, 2025 •

edited

Loading

Uh oh!

graphite-app bot commented Dec 3, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

feat(gas): add batch listen #3478

feat(gas): add batch listen #3478

Uh oh!

Conversation

MasterPtato commented Nov 18, 2025

Uh oh!

vercel bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MasterPtato commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

claude bot commented Nov 18, 2025

Pull Request Review: Batch Listen Feature

Summary

Positive Aspects

Architecture & Design

Code Quality

Critical Issues

1. Breaking Change: Macro-generated Listen Implementation

2. Inconsistent Limit Behavior

3. Missing Signal ID Tracking

4. Data Consistency Risk in History

Moderate Issues

5. Error Context Loss

6. Metrics Removed Without Explanation

7. Unused Loop Location Parameter

Minor Issues & Suggestions

8. Display Implementation Inefficiency

9. Naming: last_attempt vs last_try

10. Test Coverage

Security Considerations

11. Resource Exhaustion Risk

Performance Considerations

Positive

Potential Concerns

Documentation & Style

Recommendations

High Priority

Medium Priority

Low Priority

Conclusion

Uh oh!

claude bot commented Nov 18, 2025

PR Review: feat(gas): add batch listen

Summary

Code Quality & Best Practices

Potential Bugs

Performance Considerations

Security Concerns

Test Coverage

Architecture & Design

Summary & Recommendations

Uh oh!

claude bot commented Nov 18, 2025

Code Review

Summary

Architecture Changes

Code Quality

Potential Issues

1. EventType Mismatch in insert::signals_event (packages/gasoline/src/db/kv/keys/history.rs:1645)

2. Missing Validation on Signal Count

3. Removed CustomListener Without Clear Migration Path

4. Potential Performance Issue: Sort After Parallel Fetch

Performance Considerations

Security

Test Coverage

Additional Observations

Recommendations

Verdict

Uh oh!

claude bot commented Nov 19, 2025

PR Review: Add Batch Listen

Overview

Strengths

Concerns

Security & Safety

Conclusion

Uh oh!

vercel bot commented Nov 18, 2025 •

edited

Loading

MasterPtato commented Nov 18, 2025 •

edited

Loading

1. Breaking Change: Macro-generated `Listen` Implementation

9. Naming: `last_attempt` vs `last_try`

claude bot commented Nov 27, 2025 •

edited

Loading