[Bug] fixes SQS timeout

[lillo42]

Fixes #3917 SQS WaitTime/ChangeVisibilityTimeout

[claude]

PR Review: Fix SQS Timeout Issue

Summary

This PR fixes a critical bug where TimeSpan.Seconds was incorrectly used instead of TimeSpan.TotalSeconds when setting SQS wait times and visibility timeouts. This caused failures for any timeout values >= 60 seconds.

---

Positive Aspects

1. Correct Fix
The changes properly address the root cause identified in issue #3917:

• TimeSpan.Seconds returns only the seconds component (0-59), not the total duration
• Using Convert.ToInt32(timeOut.Value.TotalSeconds) correctly converts the entire duration to an integer representing total seconds

2. Comprehensive Coverage
Both affected files were fixed in src/Paramore.Brighter.MessagingGateway.AWSSQS and V4 versions. Both problematic uses were corrected: WaitTimeSeconds in ReceiveAsync and ChangeMessageVisibilityRequest in RequeueAsync.

3. Consistent Implementation
The fix uses Convert.ToInt32() consistently across all changes, which is appropriate since AWS SQS API expects integer values.

---

Concerns and Recommendations

1. Missing Test Coverage for the Bug
The existing test When_requeueing_a_message_async.cs doesn't test delays >= 60 seconds, which is where this bug manifests. The test uses the default timeout and doesn't verify the actual delay behavior.

Recommendation: Add a test case that configures a subscription with requeueDelay: TimeSpan.FromMinutes(5) or TimeSpan.FromSeconds(90), verifies the message doesn't reappear immediately, and confirms the delay is respected correctly.

2. Potential Integer Overflow
Convert.ToInt32() will throw OverflowException if TotalSeconds exceeds Int32.MaxValue. While unlikely in practice, AWS SQS has specific limits: WaitTimeSeconds 0-20 seconds and VisibilityTimeout 0-43,200 seconds (12 hours). Consider adding bounds validation to prevent runtime exceptions, provide clearer error handling, and make the AWS limits explicit in the code.

3. Code Style - Minor
The trailing newline was removed from SqsMessageConsumer.cs V4 version line 383. Consider keeping it for consistency with the rest of the codebase.

---

Security

No security concerns identified. The fix doesn't introduce vulnerabilities.

Performance

No performance impact. The change from .Seconds to .TotalSeconds with Convert.ToInt32() is negligible.

---

Summary

Approve with recommendations:

• The fix is correct and solves the reported issue
• Add test coverage for delays >= 60 seconds to prevent regression
• Consider adding bounds validation for AWS SQS limits
• Minor: Restore trailing newline in V4 file

The core fix is solid and can be merged, but adding the test coverage would significantly improve confidence and prevent future regressions of this bug.

[codescene-delta-analysis]

Gates Passed
4 Quality Gates Passed

See analysis details in CodeScene

Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.

[claude]

Code Review

Summary

This PR fixes a critical bug where TimeSpan.Seconds was incorrectly used instead of TimeSpan.TotalSeconds when setting SQS visibility timeout and wait time parameters. This caused message delays to fail for values ≥ 60 seconds.

✅ Positive Aspects

1. Critical Bug Fix: The fix correctly addresses the root cause identified in issue SqsMessageConsumer uses TimeSpan.Seconds instead of TotalSeconds causing incorrect requeue delay for values ≥ 60s #3917. Using TimeSpan.Seconds only returns the seconds component (0-59), not the total duration.

2. Consistent Fix: The change is applied consistently across both affected files:

   • src/Paramore.Brighter.MessagingGateway.AWSSQS/SqsMessageConsumer.cs
   • src/Paramore.Brighter.MessagingGateway.AWSSQS.V4/SqsMessageConsumer.cs

3. Correct Type Conversion: Using Convert.ToInt32(timeOut.Value.TotalSeconds) is appropriate since AWS SQS API expects integer values for these parameters.

4. Small, Focused Change: The PR follows good practices by making minimal changes necessary to fix the bug.

🔍 Code Quality Observations

Positive:

• The fix is straightforward and easy to verify
• No behavioral changes beyond the bug fix
• Follows the existing code style

Minor Note:

• The trailing newline was added to SqsMessageConsumer.cs in V4 (line 383), which is good for consistency with .NET conventions

⚠️ Test Coverage Concern

Issue: This PR does not include tests that would have caught this bug or verify the fix.

Looking at the existing test suite, I found tests like When_throwing_defer_action_respect_redrive.cs that test requeue functionality, but they use short delays (TimeSpan.FromMilliseconds(50)) that don't expose this bug.

Recommendation: Add a test case that verifies delays ≥ 60 seconds work correctly. For example:

// Test case suggestion
requeueDelay: TimeSpan.FromMinutes(2) // 120 seconds - would catch the bug

This aligns with the repository's TDD philosophy which emphasizes that "Ensure all new features and bug fixes include appropriate test coverage."

🔒 Security & Performance

• Security: No security concerns. The change doesn't introduce any vulnerabilities.
• Performance: No performance impact. The conversion from double to int is negligible.

📊 Impact Analysis

Before Fix:

Configuration	Expected	Actual (Bug)
30 seconds	30s	30s ✅
60 seconds	60s	0s ❌
90 seconds	90s	30s ❌
5 minutes	300s	0s ❌

After Fix:
All values now correctly use total seconds.

✅ Recommendation

APPROVE with suggestion: The fix is correct and addresses the critical bug. However, I recommend adding test coverage for delays ≥ 60 seconds to prevent regression.

Next Steps

Consider adding a regression test in a follow-up PR that:

1. Tests ReceiveAsync with timeOut ≥ 60 seconds
2. Tests RequeueAsync with delay ≥ 60 seconds

This would align with the project's testing guidelines which state: "Ensure all new features and bug fixes include appropriate test coverage."