feat: 30.2 — unified api surface#4
Conversation
- update service.proto: single Enqueue RPC with repeated messages, typed per-result errors for enqueue/ack/nack, consume uses only repeated messages field - remove batchEnqueue(), add enqueueMany() (no "batch" prefix) - enqueue() wraps single message in repeated, parses first result - ack()/nack() wrap single item in repeated, parse per-result errors - consume() uses only messages field (singular message field removed) - batcher uses unified Enqueue RPC for all batch sizes - fix drain race: track in-flight RPCs to prevent premature resolution - regenerate proto types, remove stale BatchEnqueue* generated files
There was a problem hiding this comment.
1 issue found across 34 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="proto/fila/v1/service.proto">
<violation number="1" location="proto/fila/v1/service.proto:59">
P1: Reusing protobuf field numbers across this redesign risks silent data corruption during any mixed-version deployment. Every restructured message (EnqueueRequest, EnqueueResponse, ConsumeResponse, AckRequest, NackRequest) reuses field 1 with a different type.
Protobuf best practice is to `reserved` retired field numbers and start new fields at the next available number, or introduce a new package version (e.g., `fila.v2`). If this is a hard-cut with no rolling-deploy window, consider at minimum adding `reserved` declarations for the old field numbers as documentation to prevent future collisions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -6,57 +6,137 @@ import "fila/v1/messages.proto"; | |||
| // Hot-path RPCs for producers and consumers. | |||
There was a problem hiding this comment.
P1: Reusing protobuf field numbers across this redesign risks silent data corruption during any mixed-version deployment. Every restructured message (EnqueueRequest, EnqueueResponse, ConsumeResponse, AckRequest, NackRequest) reuses field 1 with a different type.
Protobuf best practice is to reserved retired field numbers and start new fields at the next available number, or introduce a new package version (e.g., fila.v2). If this is a hard-cut with no rolling-deploy window, consider at minimum adding reserved declarations for the old field numbers as documentation to prevent future collisions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At proto/fila/v1/service.proto, line 59:
<comment>Reusing protobuf field numbers across this redesign risks silent data corruption during any mixed-version deployment. Every restructured message (EnqueueRequest, EnqueueResponse, ConsumeResponse, AckRequest, NackRequest) reuses field 1 with a different type.
Protobuf best practice is to `reserved` retired field numbers and start new fields at the next available number, or introduce a new package version (e.g., `fila.v2`). If this is a hard-cut with no rolling-deploy window, consider at minimum adding `reserved` declarations for the old field numbers as documentation to prevent future collisions.</comment>
<file context>
@@ -6,57 +6,137 @@ import "fila/v1/messages.proto";
message ConsumeResponse {
- Message message = 1; // Single message (backward compatible, used when batch size is 1)
- repeated Message messages = 2; // Batched messages (populated when server sends multiple at once)
+ repeated Message messages = 1;
}
</file context>
There was a problem hiding this comment.
Acknowledged — this is an intentional hard-cut redesign (Epic 30 API unification). Fila is pre-alpha with no rolling deployments or mixed-version scenarios. The server and all 6 SDKs are updated atomically in the same epic. Adding reserved declarations for retired field numbers is unnecessary here since there is no backward compatibility window to protect.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Summary
batchEnqueue()withenqueueMany()(no "batch" prefix)enqueue(),ack(),nack()all use the unified repeated-message proto formatconsume()uses only themessagesfield (singularmessagefield removed from proto)EnqueueRPC for all batch sizes (no more separateBatchEnqueueRPC)BatchEnqueue*generated filesTest plan
enqueueMany()tests cover multi-message, partial errors, orderingSummary by cubic
Unifies the JS SDK with the new API: single Enqueue with repeated messages and typed per-result errors across enqueue, ack, nack, and consume. Aligns with Linear Story 30.1 and fixes a batcher drain race.
New Features
ack()/nack()use repeated formats with per-item results;MESSAGE_NOT_FOUNDmaps toMessageNotFoundError.consume()reads onlymessages[]; the batcher always calls the unified Enqueue.Migration
batchEnqueue()withenqueueMany(). Update types fromBatchEnqueueResulttoEnqueueResult.enqueue(),ack(),nack(), orconsume()call sites.Written for commit fe3484e. Summary will update on new commits.