Skip to content

fix: repair fibp transport integration — protocol bugs across 8 sites#8

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

fix: repair fibp transport integration — protocol bugs across 8 sites#8
vieiralucas merged 2 commits intomainfrom
fix/fibp-integration-tests

Conversation

@vieiralucas
Copy link
Copy Markdown
Member

@vieiralucas vieiralucas commented Mar 26, 2026

Summary

  • conftest: Fix TLS config field names (cert_file/key_file/ca_file) and move bootstrap_apikey to [auth] section (was misplaced in [fibp])
  • conftest: Replace gRPC-based create_queue with FIBP OP_CREATE_QUEUE — gRPC was removed from fila-server; FIBP is now the sole transport
  • fibp: Fix consume stream registration — push frames arrive with corr_id=0, not the original corr_id; register consume queue under both
  • fibp: Fix consume ack handling — server sends OP_CONSUME + empty body as initial ack; was incorrectly treated as stream-closed signal
  • fibp: Replace decode_consume_message with decode_consume_push that reads the count:u16 batch prefix the server actually sends
  • fibp: Fix encode_auth to send raw UTF-8 bytes (server expects no u16-length prefix that _encode_str was adding)
  • fibp: Fix decode_error_frame to parse raw UTF-8 (server sends plain text, not u16 code + u16-prefixed message)
  • batcher: Use _map_fibp_error for transport-level FIBP errors so auth rejection raises TransportError, not EnqueueError
  • test_batcher: Update transport failure assertion to expect TransportError (was incorrectly expecting EnqueueError for a frame-level failure)

Test plan

  • All 33 tests pass locally (python -m pytest tests/ -v)
  • 2 TLS tests skip locally (missing cryptography package) — they run in CI which installs all deps
  • CI integration tests pass with downloaded fila-server binary

🤖 Generated with Claude Code


Summary by cubic

Fixes FIBP transport to match the server protocol so consume streaming, auth, and error handling work as expected. Also switches test admin ops from gRPC to FIBP to unblock CI.

  • Bug Fixes

    • Consume: register the stream under both the request corr_id and 0; treat empty OP_CONSUME as the stream ack; decode batch pushes via decode_consume_push.
    • Auth/errors: encode_auth sends raw UTF-8 bytes; error frames parsed as plain text; map ERR_AUTH_REQUIRED/ERR_PERMISSION_DENIED to TransportError.
    • Batcher: use _map_fibp_error so transport failures (e.g., auth reject) raise TransportError; tests updated.
    • Lint: address ruff errors (import order, line length, unused imports).
  • Migration

    • Test/admin: fila-server removed gRPC; fixtures now create queues via FIBP OP_CREATE_QUEUE.
    • TLS fixture config: rename [tls] keys to cert_file/key_file/ca_file; move bootstrap_apikey to [auth].

Written for commit a74fa0e. Summary will update on new commits.

- conftest: fix tls config field names (cert_file/key_file/ca_file) and
  move bootstrap_apikey to [auth] section (was misplaced in [fibp])
- conftest: replace grpc-based create_queue with fibp op_create_queue
  (grpc was removed from fila-server; fibp is now the sole transport)
- fibp: fix consume stream registration — push frames arrive with
  corr_id=0, not the original corr_id; register consume queue under both
- fibp: fix consume ack handling — server sends op_consume + empty body
  as initial ack; was incorrectly treated as stream-closed signal
- fibp: replace decode_consume_message with decode_consume_push that
  reads the count:u16 batch prefix the server actually sends
- fibp: fix encode_auth to send raw utf-8 bytes (server expects no
  u16-length prefix that _encode_str was adding)
- fibp: fix decode_error_frame to parse raw utf-8 (server sends plain
  text, not u16 code + u16-prefixed message)
- batcher: use _map_fibp_error for transport-level fibp errors so auth
  rejection raises TransportError, not EnqueueError
- test_batcher: update transport_failure assertion to expect TransportError
  (was incorrectly expecting EnqueueError for a frame-level failure)
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.

1 issue found across 7 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="fila/fibp.py">

<violation number="1" location="fila/fibp.py:457">
P1: Enforce the single-consume-stream constraint when binding `corr_id=0`; otherwise a second `consume()` call silently steals all server-push frames from the first stream.</violation>
</file>

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

Comment thread fila/fibp.py
with self._lock:
self._consume_queues[corr_id] = cq
# Push frames always arrive with corr_id=0.
self._consume_queues[0] = cq
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Mar 26, 2026

Choose a reason for hiding this comment

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

P1: Enforce the single-consume-stream constraint when binding corr_id=0; otherwise a second consume() call silently steals all server-push frames from the first stream.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At fila/fibp.py, line 457:

<comment>Enforce the single-consume-stream constraint when binding `corr_id=0`; otherwise a second `consume()` call silently steals all server-push frames from the first stream.</comment>

<file context>
@@ -416,10 +443,18 @@ def send_request(self, frame: bytes, corr_id: int) -> Future[bytes]:
         with self._lock:
             self._consume_queues[corr_id] = cq
+            # Push frames always arrive with corr_id=0.
+            self._consume_queues[0] = cq
         with self._send_lock:
             self._sock.sendall(frame)
</file context>
Fix with Cubic

@vieiralucas vieiralucas merged commit b120bfe into main Mar 26, 2026
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