Avoid spurious STREAM_STATE_ERROR on local stream reuse; add test

[victorstewart]

PR Title
Avoid spurious CONNECTION_CLOSE (STREAM_STATE_ERROR) when app reuses a closed local stream id; return API error instead. Add deterministic regression test.

Context Diagnosis

Root cause: When the application attempts to touch a locally initiated stream ID that was previously created and then closed (i.e., stream_id < cnx->next_stream_id[...] for its type), picoquic_create_missing_streams() classified it as a peer protocol violation and called picoquic_connection_error(..., PICOQUIC_TRANSPORT_STREAM_STATE_ERROR), which closes the connection.
Code path: picoquic_create_missing_streams() in picoquic/frames.c:73
Why it’s wrong: This condition stems from local API usage (e.g., racing on picoquic_get_next_local_stream_id() without immediate stream creation), not from a peer-visible protocol error. QUIC transport errors are for peer violations. This misclassification generated spurious CONNECTION_CLOSE frames.
Correct handling: Treat “old local stream id” as a local misuse and return a local API error (PICOQUIC_ERROR_STREAM_ALREADY_CLOSED) instead of closing the connection. Keep peer validation paths unchanged and spec‑compliant.
Reference behavior: QUIC RFC 9000 §2.1 explains stream ID ordering and that out-of-order use opens intermediate streams; this change doesn’t alter that. It only fixes the local error classification.

Solution

frames.c

File: picoquic/frames.c:73
Change: In picoquic_create_missing_streams(), if stream_id < cnx->next_stream_id[STREAM_TYPE_FROM_ID(stream_id)], return NULL quietly for local usage rather than calling picoquic_connection_error(..., STREAM_STATE_ERROR).
Effect: No transport error emission for local reuse; upstream caller decides how to respond.

sender.c

File: picoquic/sender.c:65
Change: In picoquic_find_stream_for_writing(), if stream_id < cnx->next_stream_id[STREAM_TYPE_FROM_ID(stream_id)], set *ret = PICOQUIC_ERROR_STREAM_ALREADY_CLOSED and don’t try to recreate the stream.
Effect: The API path now reports a clear local error, leaving the connection alive.

Remote semantics unchanged

Example: Receiving STOP_SENDING on a receive-only stream still triggers transport STREAM_STATE_ERROR (spec‑compliant), see picoquic/frames.c:923.

Why This Is Correct (Not a Bandaid)

Proper layering: Local API misuse should not tear down the connection. Transport errors are reserved for peer-visible violations. This fix restores that separation.
Spec-aligned: Stream ID monotonicity and ordering remain. We still open intermediate streams as per QUIC. We simply avoid closing the connection for an internal race on local IDs.
Invariants preserved: next_stream_id[...] remains monotonic, picoquic_create_stream() still advances it, and remote validation remains intact.
Minimal and targeted: Only the two call sites that misclassified the local condition are adjusted; all other protocol checks remain unchanged.

Tests Added

Regression test integrated into existing stream tests:

File: picoquictest/stream0_frame_test.c:24 (include) and picoquictest/stream0_frame_test.c:425 (test body)
Name: stream_state_local_reuse_test()
Steps:

• Create minimal client cnx.
• Call picoquic_get_next_local_stream_id() twice to get same id (not reserved).
• Create the stream once to advance next_stream_id, then delete it from the tree to simulate a fully closed stream.
• Call picoquic_set_app_stream_ctx() on the old id.
• Assert: returns PICOQUIC_ERROR_STREAM_ALREADY_CLOSED, connection is not closing, and no STREAM_STATE_ERROR recorded.

Registration

• picoquic_t/picoquic_t.c:329 — Added the test to the CLI test table as stream_state_local_reuse.
• UnitTest1/unittest1.cpp:351 — Added MSVC test method TEST_METHOD(stream_state_local_reuse) calling stream_state_local_reuse_test().

Repro and Results

Build command:

• cmake -S . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DPICOQUIC_FETCH_PTLS=ON
• cmake --build build -j

Before patch (showing the bug)

• Command: ./build/picoquic_ct stream_state_local_reuse
• Output excerpt:
  quic/picoquic/quicctx.c:4772 [picoquic_connection_error_ex]: Protocol error 0x5, frame 0, reason: ?
  Fails, error: -1
• Interpretation: spurious CONNECTION_CLOSE with transport STREAM_STATE_ERROR (0x5) due to local misuse.

After patch (fixed)

• Command: ./build/picoquic_ct -n stream_state_local_reuse
• Output excerpt:
  Success.
• Interpretation: The API correctly returns PICOQUIC_ERROR_STREAM_ALREADY_CLOSED, connection remains alive, test passes.

Full suite:

• ctest --test-dir build -j4 --output-on-failure
• Result: All tests pass (including this regression).

Files Changed

• picoquic/frames.c:73

  • Return NULL for old local stream-id instead of raising STREAM_STATE_ERROR.

• picoquic/sender.c:65

  • Return PICOQUIC_ERROR_STREAM_ALREADY_CLOSED when local id < next_stream_id.

• picoquictest/stream0_frame_test.c:24

  • Include picoquictest_internal.h and add stream_state_local_reuse_test() at :425.

• picoquic_t/picoquic_t.c:329

  • Add test entry { "stream_state_local_reuse", stream_state_local_reuse_test },

• UnitTest1/unittest1.cpp:351

  • Add TEST_METHOD(stream_state_local_reuse) calling stream_state_local_reuse_test().

• picoquictest/picoquictest.h:269

  • Declare int stream_state_local_reuse_test();

PR Summary (for Review)

• Fix spurious CONNECTION_CLOSE with STREAM_STATE_ERROR when app reuses a closed local stream ID by:
  • Adjusting stream creation to not escalate local “old id” condition into transport error.
  • Returning PICOQUIC_ERROR_STREAM_ALREADY_CLOSED from the sender path for clarity.
• Add a deterministic regression test integrated with existing stream tests and both test runners.
• Verified: test fails on unpatched code (with STREAM_STATE_ERROR) and passes with fix; full suite green.

How To Validate Locally

Configure and build:

• cmake -S . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DPICOQUIC_FETCH_PTLS=ON
• cmake --build build -j

Run only the new test:

• ./build/picoquic_ct stream_state_local_reuse

Run whole suite:

• ctest --test-dir build -j4 --output-on-failure

[huitema]

Close #1937

[huitema]

This PR was generated using the new Codex extension that OpenAI released last week. The fix to issue #1937 turns out to be simple, but the key is that it took only a few prompts to generate the code and the tests. That's an example of the productivity gains that can be obtained from good AI systems!

[victorstewart]

took maybe 10 minutes of back and forth, 3 or 4 prompts. but could easily be reduced to 1 prompt and maybe 4 minutes of reasoning by writing a detailed xml-formatted spec / recipe file for the agent to always follow/execute when fixing a Github Issue "the picoquic way"... like explain the bug, provide evidence and justifications for why this is the correct fix, write a test case to deterministically trigger the bug integrated into the existing test framework, then write your patch and prove your patch solves the bug and provide the log outputs, then generate a PR in xyz format and push it to the master.

and at least on max reasoning mode + full access to the filesystem and Internet, it's able to do all of this work, build the binaries, run them, ingest the outputs, iterate until perfect.. then generate and push the PR itself.