Skip to content

fix: integration test readiness for fibp#7

Merged
vieiralucas merged 3 commits intomainfrom
fix/fibp-integration-tests
Mar 26, 2026
Merged

fix: integration test readiness for fibp#7
vieiralucas merged 3 commits intomainfrom
fix/fibp-integration-tests

Conversation

@vieiralucas
Copy link
Copy Markdown
Member

@vieiralucas vieiralucas commented Mar 26, 2026

Summary

  • Wire format bugs fixed: Five distinct mismatches between the JS SDK and the Rust server's FIBP wire format were causing all integration tests to fail or produce wrong results
  • Test infrastructure fixed: Proto loading path doubling and parallel port collisions caused server startup failures in the test helper

What was broken and why

1. Auth payload (encodeAuthPayload)

JS sent u16 length + key bytes; server reads raw key bytes only. Auth always failed.

2. Error frame decoding (decodeErrorPayload)

JS expected u16 code + u16 msglen + msg; server sends raw UTF-8 message string. Error codes were garbage (e.g. 26990 = bytes "in" from "invalid"). Now decodes raw string and maps known message substrings to typed error codes.

3. Consume delivery wire format (decodeConsumeDelivery)

JS decoded id + queue + fairness_key + ...; server encodes id + fairness_key + ... (no queue field in push frame). All consumed messages had garbled data.

4. Consume stream dispatch (openStreamopenConsumeStream)

JS registered a corrId-based stream handler but server push frames use corrId = 0 with FLAG_PUSH. The subscribe ack frame (corrId = N, no FLAG_PUSH) was incorrectly interpreted as "stream ended," closing the consumer immediately. Replaced with two-phase approach: await subscribe ack as a normal request, then register a global push handler for FLAG_PUSH frames.

5. Batcher connection lifecycle

Batcher held a sync getConnSync() callback; Client.enqueue() had an await getConn() before batcher.submit(). A concurrent close()/drain() could mark the batcher closed before submit ran. Fixed by making the batcher hold an async getConn() callback so enqueue() submits synchronously without any prior await.

6. Proto resolvePath double-prefix (test/helpers.ts)

protobufjs calls resolvePath("", "/absolute/path") for the initial loadSync call. The custom resolver prepended PROTO_DIR again, producing /proto/dir//proto/dir/file.proto. Added path.isAbsolute guard.

7. Port collision in parallel test runs (vitest.config.ts)

Multiple test workers called findFreePort() simultaneously, found the same port (TOCTOU), and both tried to start fila-server on it. Added fileParallelism: false to run test files sequentially.

Test plan

  • npm run lint — passes
  • npm run typecheck — passes
  • npm test — 46/50 pass locally; 4 TLS tests skip (local binary built without rustls crypto provider, CI binary has it configured)
  • CI with downloaded pre-built binary — all tests expected to pass

🤖 Generated with Claude Code


Summary by cubic

Fixes FIBP wire format mismatches and push‑stream routing to match the Rust server, unblocking integration tests. Hardens consume streaming to avoid dropped pushes and block concurrent streams.

  • Bug Fixes
    • Auth payload: send raw UTF‑8 API key bytes (no length prefix).
    • Error frames: decode raw UTF‑8 message; map known substrings to ErrCode.
    • Consume delivery: match server push format (no queue field; id + fairness_key + ...).
    • Consume stream: register global push handler before subscribe ack; route FLAG_PUSH frames with corrId=0; expose cancel; reject concurrent streams.
    • Batcher: hold async getConn() so enqueue() queues before any await, avoiding close/drain races.
    • Test infra: guard absolute paths in protobufjs resolver; set vitest fileParallelism: false to avoid port collisions.

Written for commit 8f3541d. Summary will update on new commits.

…ructure

fix auth payload encoding: server expects raw utf-8 key bytes, not u16+bytes
fix error frame decoding: server sends raw utf-8, not u16 code + u16 len + msg
fix consume delivery decoding: server push frames omit queue field (not in wire format)
fix consume stream dispatch: push frames use corr_id=0 and flag_push routing,
  not corr_id-based routing; openConsumeStream awaits subscribe ack then
  registers global push handler for subsequent flag_push frames
fix batcher connection: batcher now holds async getConn() so enqueue() can
  submit synchronously before close()/drain() marks batcher closed
fix proto resolvepath: absolute paths were being double-prefixed with proto dir
add fileParallelism: false to vitest config to prevent port collisions when
  multiple test files start fila-server instances concurrently
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

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

2 issues found across 6 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/transport.ts">

<violation number="1" location="src/transport.ts:522">
P1: Race condition: PUSH frames received in the same TCP chunk as the subscribe ACK will be silently dropped because `this.pushHandler` is registered after the `await`.</violation>

<violation number="2" location="src/transport.ts:556">
P1: Missing guard for concurrent consume streams. Opening a second stream on the same connection silently overwrites `this.pushHandler`, causing the first stream to hang and misrouting its messages.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread src/transport.ts
Comment thread src/transport.ts Outdated
…bscribe

- throw FilaError if openConsumeStream is called while a stream is already
  active, preventing silent pushHandler overwrite
- register pushHandler before awaiting the subscribe ack so PUSH frames
  arriving in the same TCP chunk as the ACK are buffered rather than dropped
- on subscribe request failure, clear pushHandler so the connection remains
  usable for future streams
@vieiralucas vieiralucas merged commit 54b40eb into main Mar 26, 2026
2 of 3 checks passed
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