feat: PositionReconciler — auto-detect TP/SL/liquidation fills (#98)#101
Merged
fray-cloud merged 1 commit intofeat/#97-ui-overhaulfrom May 2, 2026
Merged
Conversation
…inance (#98) PR3 attaches TP/SL via algoOrder, but Binance never pushes fills back to us (no User Data Stream subscription). When TP or SL fires, the position is gone on the exchange while our DB stays at status='filled', closedAt=null — effectively a permanent ghost open position. Manual close (#97) was the only recovery path. This adds a worker-side polling reconciler that closes the loop without introducing a WebSocket dependency. Worker - New PositionReconcilerService runs every 30s (RECONCILE_INTERVAL_MS env override). Queries DB for status='filled' AND closedAt IS NULL AND mode='real' orders and reconciles each. - Per-order reconcile is extracted into a deep, dep-injected reconcileOrder() module so the algorithm is testable in isolation from Kafka/Redis/Prisma wiring. - Algorithm: getPosition() to detect vanished positions, then getIncome() windowed since order.createdAt to compute authoritative realizedPnl from REALIZED_PNL + COMMISSION + FUNDING_FEE rows. INSURANCE_CLEAR rows mean liquidation. With both TP and SL registered, sign of realizedPnl decides take_profit vs stop_loss; otherwise manual_on_exchange. - Race-safe DB write: updateMany with closedAt=null guard so a concurrent manual close can't double-emit notifications. - Auth-failure cooldown: 3 consecutive auth errors per exchange key → Redis 1h cooldown skip, so one bad key doesn't loop forever. - close-position-saga now sets closeReason on its own writes ('manual' for the happy path, 'manual_on_exchange' for already-gone reconciliation). Adapter / types - New BinanceRest.getIncome() backed by /fapi/v1/income with full IncomeType union and IncomeRecord shape exported from @coin/types. - IExchangeRest gains the matching method. Schema - Order.closeReason: String? — nullable, no default (historical rows untouched). - Manual SQL migration only (DB not reachable from this env). API - OrderResponse DTO documents closeReason. - ActivityService description suffixes Korean reason label and uses 'closed' status when closedAt is set. Frontend - New CloseReasonBadge maps each reason to a colored badge. - Order detail header shows it next to status. Dashboard recent decisions outcome map prefers closeReason over the old positive/negative-PnL guess. Tests - reconcileOrder: 8 scenarios — live position skip, TP, SL, liquidation, empty income, lock-held, race-lost (manual won the update), TP/SL unregistered → manual_on_exchange. - close-position-saga and existing api/web suites still green. Resolves #98. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Reviewer's GuideImplements a background PositionReconciler worker that periodically detects positions closed on Binance (TP/SL/liquidation/manual on exchange) by querying the futures income endpoint, reconciles corresponding open orders in the DB with race-safe locking, and propagates a structured closeReason field end-to-end through API and web UI for better status display and notifications. Sequence diagram for a single order reconciliation ticksequenceDiagram
participant T as ReconcileTimer
participant PRS as PositionReconcilerService
participant DB as PrismaOrder
participant R as Redis
participant A as BinanceRest
participant RO as reconcileOrder
participant K as KafkaProducer
T->>PRS: runOnce()
PRS->>DB: findMany(mode=real, status=filled, closedAt=null)
DB-->>PRS: openOrders
loop for each order
PRS->>R: set(reconcile:order:orderId, 1, EX 60, NX)
alt lock not acquired
R-->>PRS: null
PRS-->>PRS: skip lock_held
else lock acquired
R-->>PRS: OK
PRS->>A: getPosition(credentials, symbol)
A-->>PRS: position or null
alt position still open
PRS-->>R: del(lockKey)
PRS-->>PRS: outcome skip live_position
else position gone
PRS->>A: getIncome(credentials, symbol, window)
A-->>PRS: IncomeRecord[]
PRS->>RO: reconcileOrder(order, deps)
RO->>DB: updateMany(id, closedAt=null, set closed, realizedPnl, closeReason)
DB-->>RO: {count}
alt count == 0
RO-->>PRS: outcome skip race_lost
else count == 1
RO->>K: emit NotificationEvent(order_filled, closeReason, realizedPnl)
K-->>RO: ack
RO-->>PRS: outcome closed
end
PRS->>R: del(lockKey)
end
end
end
PRS-->>T: outcomes
ER diagram for Order table with closeReasonerDiagram
ORDER {
string id PK
string status
datetime createdAt
datetime closedAt
string realizedPnl
string closeReason
}
Class diagram for reconciler, adapters, and closeReason propagationclassDiagram
class PositionReconcilerService {
-Logger logger
-Redis redis
-Kafka kafka
-Producer producer
-timer setInterval
-boolean running
+PositionReconcilerService(prisma: PrismaService)
+onModuleInit()
+onModuleDestroy()
+runOnce() ReconcileOutcome[]
-emit(events: EventList, userId: string)
-isAuthError(err: Error) boolean
-bumpAuthFailure(exchangeKeyId: string)
}
class ReconcileOrderInput {
+string id
+string userId
+string exchange
+string symbol
+string side
+string quantity
+string filledQuantity
+string entryPrice
+string tpOrderId
+string slOrderId
+string exchangeKeyId
+Date createdAt
}
class ReconcileOrderDelegate {
+updateMany(where: ReconcileWhere, data: ReconcileData) UpdateCount
}
class ReconcileDeps {
+prisma: ReconcilePrisma
+redis: RedisLockClient
+getPosition(symbol: string) Position
+getIncome(opts: IncomeOpts) IncomeRecord[]
+emit(events: EventList) void
}
class ReconcileOutcome {
<<union>>
+action: string
+reason: CloseReason
+realizedPnl: string
}
class CloseReason {
<<enum>>
take_profit
stop_loss
liquidation
manual
manual_on_exchange
reconciled_unknown
}
class reconcileOrder {
+reconcileOrder(order: ReconcileOrderInput, deps: ReconcileDeps) ReconcileOutcome
}
class IExchangeRest {
<<interface>>
+getPosition(credentials: ExchangeCredentials, symbol: string) Position
+getIncome(credentials: ExchangeCredentials, opts: IncomeOpts) IncomeRecord[]
}
class BinanceRest {
+getIncome(credentials: ExchangeCredentials, opts: IncomeOpts) IncomeRecord[]
}
class IncomeRecord {
+string symbol
+IncomeType incomeType
+string income
+string asset
+number time
+string tradeId
+string tranId
+string info
}
class IncomeType {
<<enum>>
REALIZED_PNL
COMMISSION
FUNDING_FEE
INSURANCE_CLEAR
TRANSFER
WELCOME_BONUS
REFERRAL_KICKBACK
COMMISSION_REBATE
API_REBATE
CONTEST_REWARD
CROSS_COLLATERAL_TRANSFER
OPTIONS_PREMIUM_FEE
OPTIONS_SETTLE_PROFIT
INTERNAL_TRANSFER
AUTO_EXCHANGE
DELIVERED_SETTELMENT
COIN_SWAP_DEPOSIT
COIN_SWAP_WITHDRAW
POSITION_LIMIT_INCREASE_FEE
}
class OrderEntity {
+string id
+string status
+Date createdAt
+Date closedAt
+string realizedPnl
+CloseReason closeReason
}
class ActivityService {
+getRecentActivities() ActivityItem[]
-formatCloseReason(reason: string) string
}
class CloseReasonBadge {
+CloseReasonBadge(reason: CloseReason)
}
PositionReconcilerService --> ReconcileOrderInput
PositionReconcilerService --> ReconcileDeps
PositionReconcilerService --> IExchangeRest
PositionReconcilerService --> reconcileOrder
PositionReconcilerService --> OrderEntity
reconcileOrder --> ReconcileOrderInput
reconcileOrder --> ReconcileDeps
reconcileOrder --> ReconcileOutcome
reconcileOrder --> CloseReason
reconcileOrder --> IncomeRecord
BinanceRest ..|> IExchangeRest
IExchangeRest --> IncomeRecord
OrderEntity --> CloseReason
ActivityService --> OrderEntity
CloseReasonBadge --> CloseReason
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
classifywithinreconcile-order.ts, theentryPricebranch only computes and discardsdirection, which is effectively dead code—either implement the intended fallback PnL estimation or remove this block to avoid confusion. - The close-reason → label mapping is now duplicated in multiple places (
formatCloseReasonin the API,REASON_LABELin the worker, andLABELin the web component); consider centralizing this mapping (or at least the enum shape) to a shared module to avoid future drift between services. PositionReconcilerServiceconstructs its own Redis and Kafka clients instead of using Nest DI or existing shared clients; wiring these through injectable providers would make connection management, configuration, and testing more consistent with the rest of the app.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `classify` within `reconcile-order.ts`, the `entryPrice` branch only computes and discards `direction`, which is effectively dead code—either implement the intended fallback PnL estimation or remove this block to avoid confusion.
- The close-reason → label mapping is now duplicated in multiple places (`formatCloseReason` in the API, `REASON_LABEL` in the worker, and `LABEL` in the web component); consider centralizing this mapping (or at least the enum shape) to a shared module to avoid future drift between services.
- `PositionReconcilerService` constructs its own Redis and Kafka clients instead of using Nest DI or existing shared clients; wiring these through injectable providers would make connection management, configuration, and testing more consistent with the rest of the app.
## Individual Comments
### Comment 1
<location path="apps/worker-service/src/orders/reconciler/reconcile-order.ts" line_range="181-188" />
<code_context>
+ // Position closed but no income rows yet (Binance can lag a few seconds).
+ // Estimate from entry vs… we don't have a fill price here, so leave null
+ // and let the next tick refine when income lands.
+ if (order.entryPrice) {
+ const direction = (order.side as PositionSide) === 'long' ? 1 : -1;
+ void direction;
+ }
+ return { reason: 'reconciled_unknown', realizedPnl: null };
</code_context>
<issue_to_address>
**suggestion:** The `entryPrice` branch currently does nothing; either remove it or complete the estimation logic.
The `if (order.entryPrice)` block only computes `direction` and then discards it, so it’s effectively a no-op before returning `reconciled_unknown`. Either remove this block if you don’t intend to estimate PnL here, or implement the estimation logic so it doesn’t look partially implemented.
```suggestion
// Position closed but no income rows yet (Binance can lag a few seconds).
// We don't have a fill price here, so avoid guessing PnL and let the next
// tick refine this once income rows land.
return { reason: 'reconciled_unknown', realizedPnl: null };
```
</issue_to_address>
### Comment 2
<location path="apps/worker-service/src/orders/reconciler/position-reconciler.service.ts" line_range="90" />
<code_context>
+ network: (order.exchangeKey.network as 'mainnet' | 'testnet') ?? 'mainnet',
+ };
+
+ const adapter = REST_ADAPTERS[order.exchange as ExchangeId]();
+ const deps: ReconcileDeps = {
+ prisma: this.prisma,
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid instantiating a new REST adapter per order in the loop; cache per-exchange instances instead.
This creates a new `BinanceRest` for every order on every tick, which adds avoidable connection and allocation overhead when many orders are open. Since these adapters are reusable, initialize them once per exchange (e.g., via a memoized map) and reuse the instances across orders to cut down churn and improve runtime performance.
Suggested implementation:
```typescript
const exchangeId = order.exchange as ExchangeId;
let adapter = restAdapterCache[exchangeId];
if (!adapter) {
adapter = REST_ADAPTERS[exchangeId]();
restAdapterCache[exchangeId] = adapter;
}
```
To make this work efficiently and only instantiate adapters once per exchange per reconciliation run, you should also:
1. Declare a cache object in the same method, **before** the loop that iterates over `orders` (or however `order` is obtained), for example:
```ts
const restAdapterCache: Partial<Record<ExchangeId, ReturnType<(typeof REST_ADAPTERS)[ExchangeId]>>> = {};
```
This ensures every `order` in that method run shares the same `restAdapterCache`.
2. Make sure this declaration is in the appropriate scope (i.e., inside the method but outside the loop) so that it is not recreated for every `order`, but also not shared across different concurrent method executions unless that is explicitly desired.
</issue_to_address>
### Comment 3
<location path="packages/exchange-adapters/src/interfaces/exchange-rest.ts" line_range="74" />
<code_context>
+ credentials: ExchangeCredentials,
+ opts: {
+ symbol?: string;
+ incomeType?: string;
+ startTime?: number;
+ endTime?: number;
</code_context>
<issue_to_address>
**suggestion:** Tighten the `incomeType` parameter type to align with the `IncomeType` union where possible.
Since `IncomeType` is now defined in `@coin/types`, consider updating this REST interface to use `IncomeType | string` (or just `IncomeType` if arbitrary values aren’t required). This will improve compile-time checking both for `getIncome` callers and for any pattern matching on `incomeType` in the reconciler.
Suggested implementation:
```typescript
IncomeType,
} from '@coin/types';
```
```typescript
getIncome(
credentials: ExchangeCredentials,
opts: {
symbol?: string;
incomeType?: IncomeType | string;
startTime?: number;
endTime?: number;
limit?: number;
},
): Promise<IncomeRecord[]>;
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+181
to
+188
| // Position closed but no income rows yet (Binance can lag a few seconds). | ||
| // Estimate from entry vs… we don't have a fill price here, so leave null | ||
| // and let the next tick refine when income lands. | ||
| if (order.entryPrice) { | ||
| const direction = (order.side as PositionSide) === 'long' ? 1 : -1; | ||
| void direction; | ||
| } | ||
| return { reason: 'reconciled_unknown', realizedPnl: null }; |
Contributor
There was a problem hiding this comment.
suggestion: The entryPrice branch currently does nothing; either remove it or complete the estimation logic.
The if (order.entryPrice) block only computes direction and then discards it, so it’s effectively a no-op before returning reconciled_unknown. Either remove this block if you don’t intend to estimate PnL here, or implement the estimation logic so it doesn’t look partially implemented.
Suggested change
| // Position closed but no income rows yet (Binance can lag a few seconds). | |
| // Estimate from entry vs… we don't have a fill price here, so leave null | |
| // and let the next tick refine when income lands. | |
| if (order.entryPrice) { | |
| const direction = (order.side as PositionSide) === 'long' ? 1 : -1; | |
| void direction; | |
| } | |
| return { reason: 'reconciled_unknown', realizedPnl: null }; | |
| // Position closed but no income rows yet (Binance can lag a few seconds). | |
| // We don't have a fill price here, so avoid guessing PnL and let the next | |
| // tick refine this once income rows land. | |
| return { reason: 'reconciled_unknown', realizedPnl: null }; |
| network: (order.exchangeKey.network as 'mainnet' | 'testnet') ?? 'mainnet', | ||
| }; | ||
|
|
||
| const adapter = REST_ADAPTERS[order.exchange as ExchangeId](); |
Contributor
There was a problem hiding this comment.
suggestion (performance): Avoid instantiating a new REST adapter per order in the loop; cache per-exchange instances instead.
This creates a new BinanceRest for every order on every tick, which adds avoidable connection and allocation overhead when many orders are open. Since these adapters are reusable, initialize them once per exchange (e.g., via a memoized map) and reuse the instances across orders to cut down churn and improve runtime performance.
Suggested implementation:
const exchangeId = order.exchange as ExchangeId;
let adapter = restAdapterCache[exchangeId];
if (!adapter) {
adapter = REST_ADAPTERS[exchangeId]();
restAdapterCache[exchangeId] = adapter;
}To make this work efficiently and only instantiate adapters once per exchange per reconciliation run, you should also:
-
Declare a cache object in the same method, before the loop that iterates over
orders(or howeverorderis obtained), for example:const restAdapterCache: Partial<Record<ExchangeId, ReturnType<(typeof REST_ADAPTERS)[ExchangeId]>>> = {};
This ensures every
orderin that method run shares the samerestAdapterCache. -
Make sure this declaration is in the appropriate scope (i.e., inside the method but outside the loop) so that it is not recreated for every
order, but also not shared across different concurrent method executions unless that is explicitly desired.
| credentials: ExchangeCredentials, | ||
| opts: { | ||
| symbol?: string; | ||
| incomeType?: string; |
Contributor
There was a problem hiding this comment.
suggestion: Tighten the incomeType parameter type to align with the IncomeType union where possible.
Since IncomeType is now defined in @coin/types, consider updating this REST interface to use IncomeType | string (or just IncomeType if arbitrary values aren’t required). This will improve compile-time checking both for getIncome callers and for any pattern matching on incomeType in the reconciler.
Suggested implementation:
IncomeType,
} from '@coin/types'; getIncome(
credentials: ExchangeCredentials,
opts: {
symbol?: string;
incomeType?: IncomeType | string;
startTime?: number;
endTime?: number;
limit?: number;
},
): Promise<IncomeRecord[]>;
}
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
1 participant
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.
Closes #98.
PR3 + #97에서 TP/SL을 algoOrder로 부착하지만 Binance가 fill을 푸시하지 않아 (User Data Stream 미사용) DB에 ghost open position이 남는 문제. 수동 종료(#97)는 사후 임시방편이었음.
Worker
PositionReconcilerService— 30초 주기setInterval(RECONCILE_INTERVAL_MS오버라이드)reconcileOrder()— deps 주입, isolated 테스트 가능getPosition으로 포지션 사라짐 감지getIncome으로REALIZED_PNL + COMMISSION + FUNDING_FEE합산해서 정확한 실현 손익updateMany WHERE closedAt IS NULL+ Redis 락 (reconcile:order:<id>) — 수동 close와 중복 알림 방지manual/manual_on_exchange)Adapter / types
BinanceRest.getIncome()신규 (/fapi/v1/income)IncomeRecord,IncomeType타입@coin/types에 노출Schema
Order.closeReason String?컬럼 추가 + 마이그레이션take_profit | stop_loss | liquidation | manual | manual_on_exchange | reconciled_unknownAPI / Frontend
OrderResponseDTO에 closeReason 추가ActivityServicedescription에 한국어 사유 라벨 + 종료 상태 표기<CloseReasonBadge>컴포넌트Tests
reconcileOrder8 시나리오 (live/TP/SL/liquidation/empty income/lock/race/no-tpsl)🤖 Generated with Claude Code
Summary by Sourcery
Add a background position reconciliation worker that closes ghost futures positions by querying Binance income data and recording structured close reasons, and surface those reasons across API and web UI.
New Features:
Bug Fixes:
Enhancements:
Tests: