Fix/err bugs while testing #18
Merged
Conversation
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
|
Caution Review failedThe pull request is closed. WalkthroughThis PR renames Kafka topic "orders" → "engine-events", adds engine_id to Trade, updates matching-engine internals (matching, WAL, event building, logging, actor messages), adjusts Kafka client/consumer payload handling, timestamps/orders updates, and removes package manifests/config and startup logs for three services; also adds a utils workspace dep to api-gateway. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(66,134,244,0.5)
participant Client
end
rect rgba(52,199,89,0.5)
participant APIGateway
end
rect rgba(255,159,28,0.5)
participant MatchingEngine
end
rect rgba(153,102,255,0.5)
participant WAL
end
rect rgba(242,38,19,0.5)
participant Kafka
end
Client->>APIGateway: PlaceOrder (with clientTimestamp, gatewayTimestamp)
APIGateway->>MatchingEngine: PlaceOrderMsg (Order, replay chan, Err chan)
MatchingEngine->>MatchingEngine: MatchOrder => (trades, filledRestingOrders)
MatchingEngine->>WAL: append EngineEvents (including filled resting orders)
WAL-->>MatchingEngine: ack
MatchingEngine->>Kafka: emit "engine-events" (ORDER_ACCEPTED/ORDER_FILLED/etc)
Kafka-->>Consumers: delivered as raw payloads (ENGINE_EVENTS topic)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/order-service/src/controllers/order.controller.ts (2)
37-47:⚠️ Potential issue | 🟠 MajorValidate/fallback timestamps for backward compatibility.
clientTimestamp/gatewayTimestampare forwarded as-is; older callers or tests may omit them, resulting inundefinedvalues and potential engine validation failures. Consider defaulting to server time and validating range/finite values.Suggested fix
- const request: PlaceOrderRequest = { + const now = Date.now(); + const clientTimestamp = order.clientTimestamp ?? now; + const gatewayTimestamp = order.gatewayTimestamp ?? now; + if (!Number.isFinite(clientTimestamp) || clientTimestamp <= 0) { + throw new Error("Invalid clientTimestamp"); + } + + const request: PlaceOrderRequest = { clientOrderId: orderId, symbol: order.symbol, price: order.price, quantity: order.quantity, side: order.side, type: order.type, userId: order.userId, - clientTimestamp: order.clientTimestamp, - gatewayTimestamp: order.gatewayTimestamp, + clientTimestamp, + gatewayTimestamp, };
79-87:⚠️ Potential issue | 🟠 MajorAvoid leaking internal error details to clients.
Returning
err.messagedirectly can expose internal system details across place/cancel/modify flows. Prefer a stable public message while logging the full error server-side (you already logerr.message/stack).Suggested fix
- callback( - { - code: grpc.status.INTERNAL, - message: err.message, - name: "CreateOrderError", - } as grpc.ServiceError, - null, - ); + const publicMessage = "Internal server error"; + callback( + { + code: grpc.status.INTERNAL, + message: publicMessage, + name: "CreateOrderError", + } as grpc.ServiceError, + null, + );Also applies to: 128-135, 184-191
apps/matching-engine/internal/wal.go (2)
340-340:⚠️ Potential issue | 🟠 MajorResource leak:
defer file.Close()inside loop accumulates defers.Using
deferinside a loop doesn't close files immediately—all defers execute only when the function returns. If there are many WAL segments, this can exhaust file descriptors.Proposed fix: Extract to helper or close explicitly
for _, dirEntry := range entries { if dirEntry.IsDir() || !strings.HasSuffix(dirEntry.Name(), ".log") { continue } path := filepath.Join(sw.dirPath, dirEntry.Name()) - file, err := os.Open(path) - if err != nil { - return nil, err - } - defer file.Close() - - for { - // ... reading logic - } + segmentEntries, err := sw.readSegmentFile(path, from, to, &results) + if err != nil { + return nil, err + } + if segmentEntries != nil { + return segmentEntries, nil // early return case + } }Alternatively, close explicitly at the end of each loop iteration:
file, err := os.Open(path) if err != nil { return nil, err } // ... reading logic file.Close() // explicit close before next iteration
404-404:⚠️ Potential issue | 🟠 MajorSame resource leak issue:
defer file.Close()inside loop.Apply the same fix as suggested for
ReadFromTo.apps/matching-engine/internal/engine.go (2)
789-829:⚠️ Potential issue | 🟠 MajorNew order quantity ignores previously cancelled volume when
newQuantityis provided.If the order was reduced earlier,
newOrderQuantity = newQuantity - oldFilledQuantitycan resurrect cancelled size. Use the same “executed” definition as ModifyOrderInternal (Quantity - RemainingQuantity).🐛 Suggested fix
- oldRemainingQuantity := order.RemainingQuantity - oldFilledQuantity := order.FilledQuantity + oldRemainingQuantity := order.RemainingQuantity + executed := order.Quantity - oldRemainingQuantity // ... newOrderQuantity := oldRemainingQuantity if newQuantity != nil { - newOrderQuantity = *newQuantity - oldFilledQuantity + newOrderQuantity = *newQuantity - executed } if newOrderQuantity <= 0 { return nil, fmt.Errorf("nothing remaining after accounting for executed quantity") }
1138-1193:⚠️ Potential issue | 🟠 MajorAvoid dumping full order structs/user IDs to stdout during WAL replay.
These
fmt.Printlncalls include full orders and user identifiers, which is noisy and can leak sensitive data. Please gate behind a debug flag or use a structured logger with redaction.Also applies to: 1257-1297, 1318-1346
🤖 Fix all issues with AI agents
In `@apps/api-gateway/package.json`:
- Line 26: The package.json declares an unused workspace dependency
"@repo/utils"; remove the "@repo/utils": "workspace:*" entry from package.json,
update the lockfile by running your package manager (npm/yarn/pnpm install) and
run a quick repo-wide search to confirm no imports reference "@repo/utils" (so
you don't break anything). After removal, run the api-gateway build and tests
(e.g., yarn build / yarn test) to ensure nothing was relying on that package.
In `@apps/api-gateway/src/controllers/order.controller.ts`:
- Line 25: The hardcoded const userId in order.controller.ts is a security risk;
replace it by reading the authenticated user id injected by your auth middleware
(e.g., use req.userId or req.user.id) inside the same controller function where
const userId is declared, validate that it exists and return a 401/403 error if
missing, and remove the TODO; update any downstream usage in create/update/get
order handlers to use this dynamic userId and add a unit/integration test to
ensure unauthenticated requests are rejected.
In `@apps/matching-engine/internal/actor_registy.go`:
- Around line 33-40: There is a typo: log messages and the WAL replay method are
using "Replying"/"ReplyWal" instead of "Replaying"/"ReplayWal"; rename the
method ReplyWal to ReplayWal (both its definition and all call sites such as
actor.ReplyWal -> actor.ReplayWal) and update the slog.Info messages that
reference "Replying" to "Replaying" (including the start, failure and completion
messages that format sym.Name and len(actor.engine.AllOrders)); ensure the
renamed method signature and any interface implementations are updated to match.
In `@apps/matching-engine/internal/engine.go`:
- Around line 1347-1350: The ORDER_FILLED handling currently returns an error
when the order ID is missing (order, exists := a.engine.AllOrders[event.OrderId]
... return fmt.Errorf(...)), which aborts WAL replay on duplicates; change this
to be idempotent by skipping missing orders instead of returning an error—e.g.,
when exists is false, log a warning or debug message referencing event.OrderId
and continue processing (mirror ORDER_REJECTED’s continue behavior) so duplicate
or retried ORDER_FILLED events don’t abort replay; update the block in the
ORDER_FILLED handling that reads from a.engine.AllOrders to perform this
check-and-skip behavior.
In `@apps/matching-engine/internal/server.go`:
- Around line 33-34: Change the log message string passed to slog.Info from
"Request to place a order" to "Request to place an order" (the call invoking
slog.Info with the "order" field should be updated; locate the slog.Info(...)
invocation in server.go that logs the order variable).
- Line 67: Update the log message string in the slog.Info call so the grammar is
correct: change "Request for cancel a order" to "Request to cancel an order"
(locate the slog.Info(...) invocation that includes "orderId" and "symbol" in
server.go).
- Around line 83-90: Update the logging call in slog.Info used when modifying an
order: correct the message text from "Request to modify a order" to "Request to
modify an order" and make the log field keys use a consistent casing style
(e.g., change "ClientModifyId", "NewPrice", "NewQuantity" to camelCase
"clientModifyId", "newPrice", "newQuantity") so they match the existing keys
like "orderId", "symbol", "userId"; keep the same call to slog.Info and the same
field values (req.ClientModifyId, req.NewPrice, req.NewQuantity) but rename the
logged key strings to the chosen consistent style.
In `@apps/matching-engine/internal/wal.go`:
- Around line 449-452: The CRC change now appends an 8-byte little-endian
sequence (entry.GetSequenceNumber()) before computing actualChecksum, which
breaks compatibility with older WAL entries that used a single-byte sequence;
fix by adding explicit versioning to WAL entries and making the reader tolerant:
define a WAL version field/constant, update the writer to emit the new version
marker then continue using the 8-byte LE sequence (keep the existing write path
that already uses 8 bytes), and update the reader to first read the version
marker and branch—if version==old (pre-change) compute checksum using the
single-byte sequence path, if version==new compute checksum using the 8-byte
sequence as done where actualChecksum is computed—alternatively provide a
migration/purge tool to upgrade existing WAL files before deployment if you
cannot add versioning now.
In `@apps/order-trade-worker-service/src/controllers/order.controller.ts`:
- Line 58: The property mapping in order.controller.ts uses the misspelled DB
column "canelled_quantity" (seen as canelled_quantity: data.cancelledQuantity);
plan a coordinated refactor: add a Prisma schema change to rename the column to
"cancelled_quantity" and create a migration that preserves data (use a SQL
RENAME or create new column + copy + drop), update the Prisma client and
regenerate, update all service mappings (e.g., the mapping in
order.controller.ts) to use the new "cancelled_quantity" field, and track this
as a migration/refactor task to coordinate deploy order across services and
proto consumers to avoid downtime.
In `@apps/order-trade-worker-service/src/kafka.consumer.ts`:
- Around line 174-188: The ORDER_ACCEPTED and ORDER_REJECTED handlers call
orderController.createOrder using non-null assertions on gatewayTimestamp,
clientTimestamp, and engineTimestamp; instead, validate these optional fields
coming from OrderStatusEvent.decode() before passing them in: check that
data.gatewayTimestamp, data.clientTimestamp, and data.engineTimestamp are
present (or provide safe defaults) and return/log an error or skip creating the
order if any required timestamp is missing; update the code paths in the
handlers that build the createOrder payload (referencing
orderController.createOrder, ORDER_ACCEPTED, ORDER_REJECTED, and
OrderStatusEvent.decode) to remove the "!" assertions and use the
guarded/validated values.
- Around line 22-25: The code is casting JSON strings produced by
sideToJSON/orderTypeToJSON/orderStatusToJSON into Prisma enums via unsafe "as
unknown as" casts; remove those casts and instead implement explicit conversion:
create mapping functions (e.g., mapProtoSideToPrisma(side: number | string):
OrderSide, mapProtoTypeToPrisma(...): OrderType, mapProtoStatusToPrisma(...):
OrderStatus) that accept the proto numeric or JSON string output and return the
correctly typed Prisma enum values, then replace usages of
sideToJSON/orderTypeToJSON/orderStatusToJSON + casts with calls to these mapping
functions in the kafka.consumer handlers (where
sideToJSON/orderTypeToJSON/orderStatusToJSON are used) and in
order.controller.ts (replace lines using the unsafe casts with the mapping
calls) so TypeScript no longer needs unsafe casts and the values match Prisma
enum types.
In `@packages/kafka-client/src/constants.ts`:
- Line 2: Rename the misspelled constant ENGINE_ENVENTS to ENGINE_EVENTS in
packages/kafka-client's constants (update the exported symbol name
ENGINE_ENVENTS -> ENGINE_EVENTS) and update all references/usages across the
codebase to use ENGINE_EVENTS to avoid breaking consumers; ensure exports and
any imports (tests, consumers, or internal modules) are updated and run the test
suite to confirm no missing symbol errors.
In
`@packages/prisma/prisma/migrations/20260131123512_add_trade_engine_id/migration.sql`:
- Around line 1-8: The migration currently adds engine_id as NOT NULL in the
ALTER TABLE "trades" statement which will fail on non-empty tables; change the
migration to first ADD COLUMN "engine_id" TEXT NULL (nullable), run a backfill
step that populates trades.engine_id for existing rows (using the appropriate
default or lookup logic), and then create a follow-up migration that ALTER TABLE
"trades" ALTER COLUMN "engine_id" SET NOT NULL to enforce the constraint once
data is populated; reference the ALTER TABLE "trades" ADD COLUMN "engine_id"
statement when making these edits and ensure the backfill uses the same column
name and expected value source.
In `@packages/prisma/prisma/schema/trade.prisma`:
- Around line 2-3: The TypeScript definitions for Trade and TradeData are
missing the required engine_id field declared in the Prisma schema; update the
interfaces in packages/types/trade.d.ts to add engine_id: string on both the
Trade and TradeData interfaces so the fetched DB objects match the schema (and
scan for any derived types or factory functions that need the new property
included as required).
🧹 Nitpick comments (8)
apps/matching-engine/wal/SOLUSD/checkpoint.meta (1)
1-1: Consider excluding WAL checkpoint files from version control.This checkpoint metadata file appears to track WAL sequence state, which is runtime data. Committing this file could cause issues when multiple developers work on the codebase or when deploying to different environments, as it may conflict with actual runtime state.
Consider adding
apps/matching-engine/wal/to.gitignoreif this directory contains only runtime artifacts.apps/matching-engine/internal/actor_registy.go (1)
37-37: Useslog.Errororslog.Warnfor failure logs.Using
slog.Infofor error conditions makes it harder to filter and monitor failures. Consider using an appropriate log level.Proposed fix
- slog.Info(fmt.Sprintf("Replying the %s orderbook Failed. Error: %s", sym.Name, err.Error())) + slog.Error(fmt.Sprintf("Replaying the %s orderbook Failed", sym.Name), "error", err)apps/matching-engine/internal/wal.go (2)
331-331: Usestrings.HasSuffixinstead ofstrings.Containsfor stricter filtering.
strings.Contains(name, ".log")could match unintended files likebackup.log.bakordebug.log.old. Usingstrings.HasSuffix(name, ".log")ensures only actual.logfiles are processed.Proposed fix
- if dirEntry.IsDir() || !strings.Contains(dirEntry.Name(), ".log") { + if dirEntry.IsDir() || !strings.HasSuffix(dirEntry.Name(), ".log") {
394-394: Apply the sameHasSuffixfix here for consistency.Proposed fix
- if entry.IsDir() || !strings.Contains(entry.Name(), ".log") { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".log") {apps/matching-engine/internal/server.go (1)
95-103: Consider consistent log field naming.Error log field names mix casing conventions:
orderId(camelCase) vsClientModifyId,NewPrice,NewQuantity(PascalCase). Using consistent camelCase throughout would improve log parsing and querying.Proposed fix
slog.Error("Failed to modify order", "orderId", req.OrderId, "symbol", req.Symbol, "userId", req.UserId, - "ClientModifyId", req.ClientModifyId, - "NewPrice", req.NewPrice, - "NewQuantity", req.NewQuantity, + "clientModifyId", req.ClientModifyId, + "newPrice", req.NewPrice, + "newQuantity", req.NewQuantity, "error", err, )packages/kafka-client/src/index.ts (1)
60-68: Avoid@ts-expect-errorfor the unusedschemaparameter.It’s brittle because it fails once the compiler stops reporting an error; a no-op usage keeps the API stable without suppression.
🔧 Suggested adjustment
async subscribe<T, X>( groupId: string, topic: string, handler: (message: T, topic: string, partition: number, headers: X) => Promise<void>, - // eslint-disable-next-line `@typescript-eslint/ban-ts-comment` - // `@ts-expect-error` - // eslint-disable-next-line `@typescript-eslint/no-unused-vars` schema?: z.ZodTypeAny, ) { + void schema; // keep for API compatibility until removedapps/matching-engine/internal/utils.go (1)
56-85: Consider fixing theRemainingQuantiytypos in parameter names for clarity.apps/order-trade-worker-service/src/kafka.consumer.ts (1)
48-52: Use the topic parameter in logs for accuracy and maintainability.The hard-coded log message "Consumed message from 'orders' topic:" doesn't reflect the actual topic (
engine-events). Use thetopicparameter to keep logs accurate:Suggested improvement
- this.logger.info("Consumed message from 'orders' topic:", { topic, partition }); + this.logger.info(`Consumed message from '${topic}' topic:`, { topic, partition });
packages/prisma/prisma/migrations/20260131123512_add_trade_engine_id/migration.sql
Show resolved
Hide resolved
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 by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.