fix(trading): close 3 race / lifecycle bugs in adopt-then-release path
1. recovery.py rehydrated adopted trades with no System-2 ladder ───────────────────────────────────────────────────────────────── On backend restart, recovery resolved stop_ladder / derisk_ladder / addon_ladder / peak_trail from trigger_post.category. Adopted trades have trigger_post_id=NULL (the user opened on HL manually, there's no source post) so all four resolved to None and the trade restarted as a vanilla "stop_loss + max_hold" position — the entire System-2 ladder logic silently disappeared across any restart. Fix: detect System-2 from the row itself (sys2_mode IS NOT NULL falls back to the adopted-category constant), then rebuild the full ladder set from frozen leverage + mode. derisk_steps_done / addon_steps_done / peak_gain_pct on the row let us pick up exactly where we left off. 2. close_and_finalize raced with release_management ───────────────────────────────────────────────────────────────── on_price_tick snapshots _watched, evaluates per trade, appends to a triggered list, then spawns _fire_close as an async task. If release_management runs between the snapshot and _fire_close firing, close_and_finalize would still close the HL position the user just took back manual control of. Fix: atomic claim now requires released_at IS NULL by default. New `force=True` parameter on close_and_finalize bypasses the guard for the explicit user-close API (manual_close passes it) so a deliberate user click still works on a released trade. 3. partial_derisk + pyramid_add raced with release_management ───────────────────────────────────────────────────────────────── Same race shape: a price tick captured before release could still trigger a partial reduce-only close, or pyramid INTO a position the user is now driving themselves. Fix: both functions return idempotent-success if released_at IS NOT NULL — match the closed_at handling we already had. Tests still 64/64 + preflight + smoke all green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -316,6 +316,9 @@ async def manual_close(
|
|||||||
# close_and_finalize handles BOTH paper and live branches internally.
|
# close_and_finalize handles BOTH paper and live branches internally.
|
||||||
from app.services.bot_engine import close_and_finalize
|
from app.services.bot_engine import close_and_finalize
|
||||||
leverage = trade.leverage if trade.leverage is not None else sub.leverage
|
leverage = trade.leverage if trade.leverage is not None else sub.leverage
|
||||||
|
# force=True so an explicit user close still works on a released trade
|
||||||
|
# (the released_at guard exists to stop AUTOMATED paths from racing
|
||||||
|
# past a release; a manual click is the user being explicit).
|
||||||
await close_and_finalize(
|
await close_and_finalize(
|
||||||
trade_id=trade.id,
|
trade_id=trade.id,
|
||||||
api_key=api_key,
|
api_key=api_key,
|
||||||
@@ -323,6 +326,7 @@ async def manual_close(
|
|||||||
asset=trade.asset,
|
asset=trade.asset,
|
||||||
wallet=wallet,
|
wallet=wallet,
|
||||||
reason="manual",
|
reason="manual",
|
||||||
|
force=True,
|
||||||
)
|
)
|
||||||
|
|
||||||
closed = (await db.execute(select(BotTrade).where(BotTrade.id == trade_id))).scalar_one()
|
closed = (await db.execute(select(BotTrade).where(BotTrade.id == trade_id))).scalar_one()
|
||||||
|
|||||||
@@ -668,6 +668,11 @@ async def partial_derisk(
|
|||||||
trade = row.scalar_one_or_none()
|
trade = row.scalar_one_or_none()
|
||||||
if trade is None or trade.closed_at is not None:
|
if trade is None or trade.closed_at is not None:
|
||||||
return True # gone / already fully closed — nothing to do
|
return True # gone / already fully closed — nothing to do
|
||||||
|
if trade.released_at is not None:
|
||||||
|
# User took back manual control between tick capture and
|
||||||
|
# execution — do NOT partially reduce a position they're
|
||||||
|
# now driving themselves.
|
||||||
|
return True
|
||||||
if (trade.derisk_steps_done or 0) > step_idx:
|
if (trade.derisk_steps_done or 0) > step_idx:
|
||||||
return True # this step already executed (idempotent)
|
return True # this step already executed (idempotent)
|
||||||
if (trade.derisk_steps_done or 0) != step_idx:
|
if (trade.derisk_steps_done or 0) != step_idx:
|
||||||
@@ -757,6 +762,10 @@ async def pyramid_add(
|
|||||||
trade = row.scalar_one_or_none()
|
trade = row.scalar_one_or_none()
|
||||||
if trade is None or trade.closed_at is not None:
|
if trade is None or trade.closed_at is not None:
|
||||||
return (True, None, None)
|
return (True, None, None)
|
||||||
|
if trade.released_at is not None:
|
||||||
|
# User took back manual control — never pyramid into a
|
||||||
|
# position they're now managing themselves.
|
||||||
|
return (True, None, None)
|
||||||
if (trade.derisk_steps_done or 0) > 0:
|
if (trade.derisk_steps_done or 0) > 0:
|
||||||
return (True, None, None) # went underwater — no pyramiding
|
return (True, None, None) # went underwater — no pyramiding
|
||||||
if (trade.addon_steps_done or 0) > step_idx:
|
if (trade.addon_steps_done or 0) > step_idx:
|
||||||
@@ -840,28 +849,44 @@ async def close_and_finalize(
|
|||||||
asset: str,
|
asset: str,
|
||||||
wallet: str,
|
wallet: str,
|
||||||
reason: str = "max_hold",
|
reason: str = "max_hold",
|
||||||
|
force: bool = False,
|
||||||
) -> None:
|
) -> None:
|
||||||
"""
|
"""
|
||||||
Close a live HL position and write exit_price/pnl to BotTrade.
|
Close a live HL position and write exit_price/pnl to BotTrade.
|
||||||
Race-safe: a conditional UPDATE `closed_at = now WHERE closed_at IS NULL`
|
Race-safe: a conditional UPDATE `closed_at = now WHERE closed_at IS NULL`
|
||||||
lets exactly one caller win; losers return silently. Also wraps in a
|
lets exactly one caller win; losers return silently. Also wraps in a
|
||||||
per-trade asyncio.Lock to prevent us even *attempting* the HL API call twice.
|
per-trade asyncio.Lock to prevent us even *attempting* the HL API call twice.
|
||||||
|
|
||||||
|
`force=True` overrides the released_at guard — only the explicit user
|
||||||
|
close path (manual_close API) should pass it. Automated paths (tp/sl
|
||||||
|
monitor, max-hold, time-stop, reconciler) must respect release.
|
||||||
"""
|
"""
|
||||||
async with _lock_for(trade_id):
|
async with _lock_for(trade_id):
|
||||||
async with AsyncSessionLocal() as db:
|
async with AsyncSessionLocal() as db:
|
||||||
try:
|
try:
|
||||||
now_naive = datetime.now(timezone.utc).replace(tzinfo=None)
|
now_naive = datetime.now(timezone.utc).replace(tzinfo=None)
|
||||||
|
|
||||||
# Atomic claim: only one caller sets closed_at.
|
# Atomic claim: only one caller sets closed_at. For automated
|
||||||
claim = await db.execute(
|
# callers, ALSO require released_at IS NULL — a price tick
|
||||||
|
# captured BEFORE release_management ran would otherwise be
|
||||||
|
# able to close an HL position the user just took back
|
||||||
|
# control of. The manual user-close endpoint sets force=True
|
||||||
|
# because the user explicitly wants the position gone
|
||||||
|
# regardless of release state.
|
||||||
|
claim_stmt = (
|
||||||
update(BotTrade)
|
update(BotTrade)
|
||||||
.where(BotTrade.id == trade_id)
|
.where(BotTrade.id == trade_id)
|
||||||
.where(BotTrade.closed_at.is_(None))
|
.where(BotTrade.closed_at.is_(None))
|
||||||
.values(closed_at=now_naive)
|
|
||||||
)
|
)
|
||||||
|
if not force:
|
||||||
|
claim_stmt = claim_stmt.where(BotTrade.released_at.is_(None))
|
||||||
|
claim = await db.execute(claim_stmt.values(closed_at=now_naive))
|
||||||
await db.commit()
|
await db.commit()
|
||||||
if claim.rowcount == 0:
|
if claim.rowcount == 0:
|
||||||
return # someone else closed it
|
# Either someone else closed it, or (automated path) the
|
||||||
|
# user released it between the price-tick capture and
|
||||||
|
# this close attempt. Either way: bail without touching HL.
|
||||||
|
return
|
||||||
|
|
||||||
# Reload the row we just claimed
|
# Reload the row we just claimed
|
||||||
result = await db.execute(select(BotTrade).where(BotTrade.id == trade_id))
|
result = await db.execute(select(BotTrade).where(BotTrade.id == trade_id))
|
||||||
|
|||||||
+28
-17
@@ -90,6 +90,30 @@ async def rehydrate_open_trades() -> None:
|
|||||||
select(Post).where(Post.id == t.trigger_post_id)
|
select(Post).where(Post.id == t.trigger_post_id)
|
||||||
)
|
)
|
||||||
trigger_post = post_res.scalar_one_or_none()
|
trigger_post = post_res.scalar_one_or_none()
|
||||||
|
# System-2 detection. Two paths reach here:
|
||||||
|
# (a) Auto-opened (legacy): trigger_post.category is set
|
||||||
|
# to e.g. "btc_bottom_reversal_long" → look up via
|
||||||
|
# the post's category.
|
||||||
|
# (b) Adopted (current): trigger_post_id is NULL because
|
||||||
|
# the user opened on HL manually. The trade IS a
|
||||||
|
# System-2 trade — sys2_mode is non-NULL on the row —
|
||||||
|
# but it has no source post to read category from.
|
||||||
|
# Fall back to the adopted-category constant so the
|
||||||
|
# ladder still rebuilds correctly on restart.
|
||||||
|
#
|
||||||
|
# Bug this guards: before this fix, adopted trades restarted
|
||||||
|
# with stop_ladder=None / derisk=None / addon=None /
|
||||||
|
# peak_trail=None — the entire System-2 ladder logic was
|
||||||
|
# silently lost across any backend restart, downgrading the
|
||||||
|
# trade to a plain stop_loss + max_hold position.
|
||||||
|
_cat_for_ladders = (
|
||||||
|
(trigger_post.category if trigger_post else None)
|
||||||
|
or (
|
||||||
|
"btc_bottom_reversal_long"
|
||||||
|
if t.sys2_mode is not None else None
|
||||||
|
)
|
||||||
|
)
|
||||||
|
_stop_ladder = _get_stop_ladder(_cat_for_ladders)
|
||||||
register_trade(
|
register_trade(
|
||||||
trade_id=t.id,
|
trade_id=t.id,
|
||||||
wallet=t.wallet_address,
|
wallet=t.wallet_address,
|
||||||
@@ -109,34 +133,21 @@ async def rehydrate_open_trades() -> None:
|
|||||||
else (trigger_post.invalidation_price if trigger_post else None)
|
else (trigger_post.invalidation_price if trigger_post else None)
|
||||||
),
|
),
|
||||||
min_hold_until_ts=t.eff_min_hold_until_ts,
|
min_hold_until_ts=t.eff_min_hold_until_ts,
|
||||||
stop_ladder=_get_stop_ladder(
|
stop_ladder=_stop_ladder,
|
||||||
trigger_post.category if trigger_post else None
|
|
||||||
),
|
|
||||||
# Restore staged de-risk: rebuild the ladder from the trade's
|
|
||||||
# FROZEN leverage and skip steps already executed before the
|
|
||||||
# restart (derisk_steps_done is persisted on the row).
|
|
||||||
# Restore staged de-risk/pyramid/peak-trail with the trade's
|
|
||||||
# FROZEN risk mode + leverage, skipping steps already executed
|
|
||||||
# before the restart (persisted on the row).
|
|
||||||
derisk_ladder=(
|
derisk_ladder=(
|
||||||
_sys2_derisk_ladder(t.leverage or 1, t.sys2_mode)
|
_sys2_derisk_ladder(t.leverage or 1, t.sys2_mode)
|
||||||
if _get_stop_ladder(trigger_post.category if trigger_post else None)
|
if _stop_ladder else None
|
||||||
else None
|
|
||||||
),
|
),
|
||||||
derisk_done=(t.derisk_steps_done or 0),
|
derisk_done=(t.derisk_steps_done or 0),
|
||||||
addon_ladder=(
|
addon_ladder=(
|
||||||
_sys2_addon_ladder(t.sys2_mode)
|
_sys2_addon_ladder(t.sys2_mode)
|
||||||
if _get_stop_ladder(trigger_post.category if trigger_post else None)
|
if _stop_ladder else None
|
||||||
else None
|
|
||||||
),
|
),
|
||||||
addon_done=(t.addon_steps_done or 0),
|
addon_done=(t.addon_steps_done or 0),
|
||||||
# Restore the monotonic peak so a pyramided / in-profit trade
|
|
||||||
# doesn't fall back to the underwater de-risk regime on restart.
|
|
||||||
initial_peak=(t.peak_gain_pct or 0.0),
|
initial_peak=(t.peak_gain_pct or 0.0),
|
||||||
peak_trail=(
|
peak_trail=(
|
||||||
_sys2_peak_trail(t.sys2_mode)
|
_sys2_peak_trail(t.sys2_mode)
|
||||||
if _get_stop_ladder(trigger_post.category if trigger_post else None)
|
if _stop_ladder else None
|
||||||
else None
|
|
||||||
),
|
),
|
||||||
grow_mode=bool(getattr(t, "grow_mode", False)),
|
grow_mode=bool(getattr(t, "grow_mode", False)),
|
||||||
)
|
)
|
||||||
|
|||||||
Reference in New Issue
Block a user