stack size config

[navicore]

⏺ Done. Coroutine stack size is now configurable via SEQ_STACK_SIZE environment variable:

Default: 1MB

./my-program

Custom: 2MB

SEQ_STACK_SIZE=2097152 ./my-program

Simple change, good principle. Also fixed the comment in tcp_test.rs which said "1MB" but was actually 128KB (0x20000).

[claude]

PR Review: Stack Size Configuration

Summary

This PR introduces configurable coroutine stack size via the SEQ_STACK_SIZE environment variable, with a sensible default of 1MB. The change is well-implemented and follows good software engineering principles.

Strengths

1. Good Design Principle: Making stack size configurable without recompilation is excellent for operational flexibility. Users can tune based on their workload without rebuilding.

2. Clear Documentation: The README addition with a table format is helpful and includes practical examples.

3. Sensible Default: 1MB is a reasonable default that balances safety with May internal limitations.

4. Bug Fix Included: Correcting the comment in tcp_test.rs improves code clarity.

5. Error Handling: Provides graceful fallback for invalid values.

Suggestions for Improvement

1. Add Validation for Stack Size (scheduler.rs:68-71)

The current implementation silently falls back to default for invalid values. Consider adding validation with warnings for values that are too small or too large. Users should know if their configuration is being ignored.

2. Add Test Coverage

No tests verify the new environment variable behavior. Consider adding tests for valid values, invalid values, and boundary conditions. Note: The Once::call_once pattern makes testing difficult - consider extracting configuration logic into a testable function.

3. Documentation Enhancement

Add to the comments what happens with invalid values.

4. Consistency Check

The tcp_test.rs now hardcodes 0x100000. Consider using the DEFAULT_STACK_SIZE constant for consistency, or document why it needs a specific hardcoded value.

Security Assessment

No security concerns. Environment variable parsing is safe with no shell injection risk and graceful handling of invalid input.

Performance Considerations

No performance impact. Configuration happens once during initialization with negligible one-time cost.

Potential Issues

1. Silent Failures: Invalid configurations are silently ignored.
2. Testing Difficulty: Once::call_once makes testing different configurations difficult.
3. May Internal Limits: Not validated - users setting 64MB will get a panic instead of helpful error.

Test Coverage

Missing test coverage for the new functionality.

Recommendation

APPROVE with suggestions. This is a solid change that improves operational flexibility. The core implementation is sound and the default behavior is unchanged, making this a safe, backward-compatible enhancement.

Nice Touches

• Fixed the misleading comment in tcp_test.rs
• Added comprehensive README documentation
• Used const for default value
• Detailed commit message

Great work!

[claude]

Code Review for PR #71: Stack Size Configuration

Summary

This PR adds runtime configuration for coroutine stack size via the SEQ_STACK_SIZE environment variable. The implementation is clean, well-tested, and addresses a real need for users hitting stack overflow in deeply nested calls.

Strengths

1. Clean Implementation: The parse_stack_size function follows good Rust practices with proper error handling and fallback behavior.

2. Excellent Documentation: Clear inline comments explaining the rationale, updated README with a proper configuration table, and good examples provided.

3. Comprehensive Test Coverage: Tests cover all edge cases - valid numeric inputs, None/missing values, zero (invalid), non-numeric strings, negative numbers, and floating-point numbers.

4. Bug Fix Bonus: Fixed the incorrect comment in tcp_test.rs (said 1MB but was actually 128KB).

5. User-Friendly Error Handling: Invalid values print warnings to stderr and fall back to defaults rather than panicking.

Code Quality & Best Practices

Good:

• Proper use of const for DEFAULT_STACK_SIZE
• Idiomatic Rust pattern matching
• Appropriate use of eprintln! for warnings
• Function is properly testable (pure function taking Option)

Potential Issues

None found! The implementation handles all edge cases appropriately:

• Zero is rejected (good - would be meaningless)
• Negative numbers are rejected (good - would cause panic with usize parsing)
• Non-numeric strings are handled gracefully
• The warning messages are informative

Performance Considerations

Excellent:

• Parsing happens once during initialization (SCHEDULER_INIT.call_once)
• No runtime overhead after initialization
• No unnecessary allocations in the hot path

Security Concerns

Low Risk:

• Environment variable parsing is safe (no command injection risk)
• Invalid inputs fall back to safe defaults
• Users can set arbitrarily large values, but May library has internal maximum limits (mentioned in comments)

Optional Consideration: You might want to add an upper bound check to provide a better error message before May panics with ExceedsMaximumSize. However, this is optional since the comment already mentions May internal maximum and users tuning this are likely advanced users.

Test Coverage

Excellent coverage. All paths are tested: valid positive integers, boundary cases, None/missing env var, zero, non-numeric strings, negative numbers, empty strings, and floating-point numbers.

Note: Tests that print warnings to stderr will show output during cargo test. This is fine and expected.

Documentation

Comprehensive:

• README updated with clear configuration table
• Example usage provided
• Inline code comments explain the rationale
• Function documentation explains behavior

Final Verdict

Approved! This is a well-implemented, properly tested feature that adds useful configurability without complexity. The code follows Rust best practices, handles edge cases gracefully, and includes excellent documentation.

Recommendation: Merge as-is.

Checklist

• Code quality and best practices: ✅
• No bugs or issues identified: ✅
• Good performance characteristics: ✅
• Secure implementation: ✅
• Comprehensive test coverage: ✅
• Excellent documentation: ✅

Great work!

[claude]

Pull Request Review: Stack Size Configuration

Overview

This PR adds runtime configurability for coroutine stack sizes via the SEQ_STACK_SIZE environment variable. The implementation is solid and well-tested. Great work on the documentation and test coverage!

---

✅ Strengths

1. Excellent Code Quality

• Clean, idiomatic Rust with proper error handling
• Well-structured parse_stack_size function with clear logic flow
• Appropriate use of eprintln\! for user-facing warnings
• Good separation of concerns

2. Comprehensive Test Coverage ⭐

• Tests cover all edge cases: valid values, None, zero, invalid strings, negative numbers, empty strings, and floats
• Test naming is clear and follows Rust conventions
• Good use of assertions with descriptive messages

3. Documentation

• Clear inline comments explaining the rationale
• Updated README with environment variable documentation in a nice table format
• Good examples provided

4. Bug Fix

• Fixed the misleading comment in tcp_test.rs:16 (said "1MB" but was 128KB/0x20000) ✅

---

🔍 Code Review Findings

Minor Issues

1. Consider usize::MAX Boundary (scheduler.rs:57-78)

While the implementation correctly handles zero and negative values, extremely large values could theoretically cause issues with Mays internal limits. The code mentions May has an internal maximum (64MB causes ExceedsMaximumSize panic).

Suggestion: Consider adding an upper bound check to provide a better error message than Mays panic.

Status: Enhancement suggestion, not critical since May will panic anyway ⚠️

Performance Considerations

2. Environment Variable Read Timing (scheduler.rs:95)

The environment variable is read during SCHEDULER_INIT (via Once), which is good - it is only read once at first scheduler initialization.

Observation: If the user sets SEQ_STACK_SIZE after the first scheduler call, it will not take effect. This is acceptable behavior, but might be worth documenting.

Documentation addition for README:

**Note**: `SEQ_STACK_SIZE` must be set before the first coroutine is spawned.

Status: Documentation enhancement 📝

---

🔒 Security Considerations

✅ No Security Issues Found

• Input validation is proper (rejects zero, non-numeric values)
• No buffer overflows or memory safety issues
• Warning messages do not leak sensitive information
• Environment variable parsing is safe

---

🎯 Best Practices Assessment

Category	Rating	Notes
Code Quality	⭐⭐⭐⭐⭐	Clean, idiomatic Rust
Test Coverage	⭐⭐⭐⭐⭐	Comprehensive edge case testing
Documentation	⭐⭐⭐⭐	Good, could add timing note
Error Handling	⭐⭐⭐⭐⭐	Proper warnings, graceful fallbacks
Performance	⭐⭐⭐⭐⭐	Minimal overhead (read once)
Security	⭐⭐⭐⭐⭐	No concerns

---

📋 Recommendations

Must Address

None - the PR is ready to merge as-is! ✅

Should Consider

1. Add upper bound validation for Mays 64MB limit (prevents panic with better error message)
2. Document that SEQ_STACK_SIZE must be set before first scheduler initialization

Nice to Have

1. Consider adding a test that validates the stack size is actually applied (integration test)
2. Add example usage in examples/ directory demonstrating when to increase stack size

---

🎉 Summary

This is a high-quality PR that adds useful functionality with excellent test coverage and documentation. The code follows Rust best practices and integrates cleanly with the existing codebase. The bug fix in tcp_test.rs is a nice bonus.

Recommendation: ✅ APPROVE with minor suggestions

The suggestions above are enhancements, not blockers. The PR is ready to merge in its current state.

---

Review conducted following repository conventions and Rust best practices