feat(gas): add batch listen #3478
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Pull Request Review: Batch Listen FeatureThis 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. SummaryChanges: 1,215 additions, 732 deletions across 16 files Positive AspectsArchitecture & Design
Code Quality
Critical Issues1. Breaking Change: Macro-generated
|
MasterPtato
force-pushed
the
11-17-feat_gas_add_batch_listen
branch
from
25ece3d to
6ff29f6
Compare
MasterPtato
force-pushed
the
11-17-chore_gas_add_test_for_history_with_holes
branch
from
5c9b75f to
0471792
Compare
PR Review: feat(gas): add batch listenSummaryThis 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 PracticesStrengths:
Issues & Suggestions:
Potential Bugs
Performance ConsiderationsImprovements:
Concerns:
Security Concerns
Test CoverageMissing:
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 & DesignPositive:
Questions:
Summary & RecommendationsThis is a well-structured PR that adds valuable batching capabilities to the workflow engine. However, it needs: Must Fix:
Should Fix:
Nice to Have:
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 |
Code ReviewSummaryThis 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:
Code Quality✅ Strengths:
Potential Issues1. 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 Recommended fix: EventType::Signals, // Use the new batch signal type2. Missing Validation on Signal CountThe 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 })
}
}3. Removed CustomListener Without Clear Migration PathThe PR removes the
If any users were using
4. Potential Performance Issue: Sort After Parallel FetchIn .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
Recommendation: Consider limiting the total fetch earlier or documenting the tradeoff. Performance Considerations✅ Good:
Security✅ No obvious security concerns
Test Coverage❌ Critical Gap: No Tests for Batch Listening The PR adds significant new functionality but I could not find any tests for:
Recommendation: Add tests covering:
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
RecommendationsMust Fix:
Should Consider:
Nice to Have:
VerdictThis 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. |
No description provided.