Conversation
1. Serialization moved to seq-runtime - TypedValue, TypedMapKey, SerializeError, ValueSerialize now live in the core runtime where any extension can use them 2. Value ↔ TypedValue conversion - Bidirectional conversion with proper error handling for non-serializable types (Quotation, Closure) 3. seq-actors updated - Now imports serialization types from seq-runtime, uses TypedValue for actor state 4. Architecture ready for extensions: seq-runtime └── TypedValue (serializable form) seq-actors → uses TypedValue for persistence seq-arrow → would use TypedValue for Arrow conversion seq-parquet → would use TypedValue for file storage The foundation is in place for Seq to interface with various external data systems through a common serializable representation.
Code Review: Serialization Support for Seq RuntimeSummaryThis PR adds a well-architected serialization layer to ✅ StrengthsArchitecture & Design
Code Quality
🔍 Issues & Concerns1. Security: Float NaN/Infinity Handling (Medium Priority)Location: Float(f64), // No validation on serialization/deserializationIssue:
Recommendation: Add validation in Value::Float(v) => {
if !v.is_finite() {
return Err(SerializeError::InvalidData(
format!("Cannot serialize non-finite float: {}", v)
));
}
Ok(TypedValue::Float(*v))
}2. Performance: Redundant HashMap→BTreeMap Conversion (Medium Priority)Location: Value::Map(map) => {
let mut typed_map = BTreeMap::new(); // O(n log n) insertions
for (k, v) in map.iter() {
// ...
typed_map.insert(typed_key, typed_value);
}
Ok(TypedValue::Map(typed_map))
}Issue: Runtime uses
Questions:
Recommendation: If order doesn't matter, use 3. Bug: Missing Map Support in
|
Changes Made
1. Float NaN/Infinity validation (must fix ✓)
Value::Float(v) => {
if !v.is_finite() {
return Err(SerializeError::NonFiniteFloat(*v));
}
Ok(TypedValue::Float(*v))
}
2. BTreeMap rationale documented (should consider ✓)
Added module-level docs explaining deterministic serialization benefits.
3. Error chain preserved (should consider ✓)
BincodeError(Box<bincode::Error>) // was: BincodeError(String)
impl std::error::Error for SerializeError {
fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { ... }
}
4. Corruption tests added (future improvement ✓)
- test_nan_not_serializable
- test_infinity_not_serializable
- test_corrupted_data_returns_error
- test_empty_data_returns_error
- test_truncated_data_returns_error
CI passes with 17 serialize tests now (was 12).
Pull Request Review: Serialization Foundation for SeqSummaryThis PR introduces a robust serialization layer for Seq runtime values, moving from seq-actors to seq-runtime where it can be shared across extensions. The implementation is well-architected and demonstrates excellent attention to detail based on prior feedback. ✅ StrengthsArchitecture & Design
Code Quality
Testing
🔍 Areas for Improvement1. Memory Safety Concern (Minor)Location: TypedValue::String(s) => Value::String(global_string(s.clone()))Issue: The comment notes strings are allocated as global strings "to ensure they outlive any strand context." However, this could lead to memory leaks if Recommendation:
2. Error Message ClarityLocation: pub fn to_map_key(&self) -> Result<TypedMapKey, SerializeError> {
match self {
...
TypedValue::Float(_) => Err(SerializeError::InvalidData(
"Float cannot be a map key".to_string(),
)),Issue: Using Recommendation: Consider adding a new error variant like 3. Test Gap: Large Nested StructuresCoverage: While there's a Recommendation: Add a test for deeply nested structures (5+ levels) to ensure:
4. Missing DocumentationLocation: The Recommendation: Add doc comments: /// Only hashable, equality-comparable types can be map keys.
/// Float is excluded due to NaN equality issues.
/// Maps, Variants, Quotations, and Closures are excluded as they're not hashable.
5. Performance ConsiderationLocation: The Consideration: This is likely acceptable given the use case (infrequent serialization for persistence/snapshots), but consider:
🔒 Security Analysis✅ Good Security Practices
|
Phase 2: Lock-Free Strand Registry
New in scheduler.rs:
- StrandSlot struct with atomic strand_id and spawn_time
- StrandRegistry with:
- Fixed-size array of slots (default 1024, configurable via SEQ_STRAND_REGISTRY_SIZE)
- Lock-free register() using CAS to claim slots
- Lock-free unregister() using CAS to clear slots
- active_strands() iterator for diagnostics
- overflow_count to track when registry fills up
- strand_registry() function for lazy global initialization
- Integration into patch_seq_strand_spawn (register on spawn, unregister on completion)
Updated diagnostics.rs:
- New [Active Strand Details] section showing:
- Registry capacity
- Overflow warnings if registry was full
- List of active strands with IDs and running duration (sorted oldest first)
- Limited to 20 strands to avoid overwhelming output
Sample SIGQUIT output now looks like:
=== Seq Runtime Diagnostics ===
Timestamp: SystemTime { ... }
[Strands]
Active: 5
Spawned: 100 (total)
Completed: 95 (total)
Peak: 12 (high-water mark)
[Active Strand Details]
Registry capacity: 1024 slots
5 strand(s) tracked:
[ 1] Strand #42 running for 15s
[ 2] Strand #67 running for 8s
[ 3] Strand #89 running for 3s
[ 4] Strand #95 running for 1s
[ 5] Strand #100 running for 0s
[Channels]
Open channels: 3
=== End Diagnostics ===
All 492+ tests pass and CI is green. The implementation maintains zero hot-path overhead - just a few atomic CAS operations per spawn/complete.
1 participant
└── TypedValue (serializable form)
seq-actors → uses TypedValue for persistence
seq-arrow → would use TypedValue for Arrow conversion
seq-parquet → would use TypedValue for file storage
The foundation is in place for Seq to interface with various external data systems through a common serializable representation.