fix: repair fibp transport integration — protocol bugs across 8 sites#8
Merged
vieiralucas merged 2 commits intomainfrom Mar 26, 2026
Merged
fix: repair fibp transport integration — protocol bugs across 8 sites#8vieiralucas merged 2 commits intomainfrom
vieiralucas merged 2 commits intomainfrom
Conversation
- 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)
There was a problem hiding this comment.
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.
| with self._lock: | ||
| self._consume_queues[corr_id] = cq | ||
| # Push frames always arrive with corr_id=0. | ||
| self._consume_queues[0] = cq |
There was a problem hiding this comment.
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cert_file/key_file/ca_file) and movebootstrap_apikeyto[auth]section (was misplaced in[fibp])create_queuewith FIBPOP_CREATE_QUEUE— gRPC was removed from fila-server; FIBP is now the sole transportcorr_id=0, not the original corr_id; register consume queue under bothOP_CONSUME+ empty body as initial ack; was incorrectly treated as stream-closed signaldecode_consume_messagewithdecode_consume_pushthat reads thecount:u16batch prefix the server actually sendsencode_authto send raw UTF-8 bytes (server expects nou16-length prefix that_encode_strwas adding)decode_error_frameto parse raw UTF-8 (server sends plain text, notu16 code + u16-prefixed message)_map_fibp_errorfor transport-level FIBP errors so auth rejection raisesTransportError, notEnqueueErrorTransportError(was incorrectly expectingEnqueueErrorfor a frame-level failure)Test plan
python -m pytest tests/ -v)cryptographypackage) — they run in CI which installs all depsfila-serverbinary🤖 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
corr_idand0; treat emptyOP_CONSUMEas the stream ack; decode batch pushes viadecode_consume_push.encode_authsends raw UTF-8 bytes; error frames parsed as plain text; mapERR_AUTH_REQUIRED/ERR_PERMISSION_DENIEDtoTransportError._map_fibp_errorso transport failures (e.g., auth reject) raiseTransportError; tests updated.rufferrors (import order, line length, unused imports).Migration
fila-serverremoved gRPC; fixtures now create queues via FIBPOP_CREATE_QUEUE.[tls]keys tocert_file/key_file/ca_file; movebootstrap_apikeyto[auth].Written for commit a74fa0e. Summary will update on new commits.