Skip to content

Comments

fix: enforce 1 MiB minimum for maxInflightBytes#129

Open
infiniteregrets wants to merge 2 commits intomainfrom
fix/enforce-min-inflight-bytes
Open

fix: enforce 1 MiB minimum for maxInflightBytes#129
infiniteregrets wants to merge 2 commits intomainfrom
fix/enforce-min-inflight-bytes

Conversation

@infiniteregrets
Copy link
Member

Summary

  • Throw S2Error with origin: "sdk" if maxInflightBytes < 1 MiB instead of silently clamping
  • Matches Rust SDK behavior (returns ValidationError, session/append.rs:118-119)
  • Aligns with team decision to standardize on returning errors for config validation

Test plan

  • Existing tests pass
  • Add test for rejected config < 1 MiB (follow-up)

🤖 Generated with Claude Code

@infiniteregrets infiniteregrets requested a review from a team as a code owner February 9, 2026 00:49
@greptile-apps
Copy link

greptile-apps bot commented Feb 9, 2026

Greptile Overview

Greptile Summary

Changed validation behavior for maxInflightBytes in RetryAppendSession constructor from silently clamping to throwing an S2Error when the value is below the 1 MiB minimum. This aligns with Rust SDK behavior and standardizes config validation across SDKs.

Key changes:

  • Replaced Math.max() clamping with explicit validation check
  • Throws S2Error with origin: "sdk" for invalid configuration
  • Error message clearly states the minimum requirement

Issues found:

  • Existing test at appendSession.test.ts:214 uses maxInflightBytes: 100 which will now throw an error instead of being clamped to 1 MiB

Confidence Score: 3/5

  • This PR requires test updates before merging due to a breaking change in validation behavior
  • The code change itself is straightforward and correctly implements the validation logic. However, it introduces a breaking change that will cause an existing test to fail. The PR description mentions adding a test for the rejected config as a follow-up, but the existing test that will break should be addressed first. The logic is sound and matches the stated goal of aligning with Rust SDK behavior.
  • The test file packages/streamstore/src/tests/appendSession.test.ts needs attention to update the test case that uses maxInflightBytes: 100

Important Files Changed

Filename Overview
packages/streamstore/src/lib/retry.ts Changed from silently clamping to throwing error for maxInflightBytes < 1 MiB. Breaks existing test using 100 bytes.

Sequence Diagram

sequenceDiagram
    participant Client
    participant RetryAppendSession
    participant S2Error
    
    Client->>RetryAppendSession: new RetryAppendSession(options)
    
    alt maxInflightBytes provided
        RetryAppendSession->>RetryAppendSession: Get maxInflightBytes from options
    else maxInflightBytes not provided
        RetryAppendSession->>RetryAppendSession: Use DEFAULT_MAX_INFLIGHT_BYTES (3 MiB)
    end
    
    alt maxInflightBytes < MIN_MAX_INFLIGHT_BYTES (1 MiB)
        RetryAppendSession->>S2Error: throw new S2Error with origin: "sdk"
        S2Error-->>Client: Error thrown
    else maxInflightBytes >= MIN_MAX_INFLIGHT_BYTES
        RetryAppendSession->>RetryAppendSession: Set maxQueuedBytes = maxInflightBytes
        RetryAppendSession-->>Client: Session created successfully
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines +653 to +658
if (maxInflightBytes < MIN_MAX_INFLIGHT_BYTES) {
throw new S2Error({
message: `maxInflightBytes must be at least ${MIN_MAX_INFLIGHT_BYTES}`,
origin: "sdk",
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change will break the existing test in appendSession.test.ts:214 which uses maxInflightBytes: 100

the test will need to be updated to use a value >= 1 MiB or to expect this error

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/streamstore/src/lib/retry.ts
Line: 653:658

Comment:
this change will break the existing test in `appendSession.test.ts:214` which uses `maxInflightBytes: 100`

the test will need to be updated to use a value >= 1 MiB or to expect this error

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant