Skip to content

Fix stale async queue pid after closePosition + add regression test#198

Merged
nialexsan merged 4 commits intonialexsan/close-positionfrom
lionel/fix-close-position-stale-async-queue
Mar 4, 2026
Merged

Fix stale async queue pid after closePosition + add regression test#198
nialexsan merged 4 commits intonialexsan/close-positionfrom
lionel/fix-close-position-stale-async-queue

Conversation

@liobrasil
Copy link
Contributor

@liobrasil liobrasil commented Mar 4, 2026

Summary

Fix stale async queue entries after closePosition so asyncUpdate() cannot panic when a previously queued position has already been removed.

Previous behavior (bug example)

Example sequence:

  1. User opens position pid=0.
  2. User deposits beyond active capacity, so part of funds are queued and pid=0 is queued in positionsNeedingUpdates.
  3. User closes pid=0 before async callbacks drain the queue.
  4. Protocol account calls asyncUpdate().

Before this fix:

  • closePosition deleted the position from self.positions.
  • The same pid could still remain in positionsNeedingUpdates.
  • asyncUpdate() could pop that stale pid and attempt to update a non-existent position, causing a panic.

Why this fix is needed

This is a production stability issue: a valid user flow (queue then close) could break async processing by introducing stale queue state. Async callbacks must keep progressing even when positions are closed between queueing and processing.

What changed

  • Updated FlowALPv0.cdc:
    • closePosition now removes its pid from positionsNeedingUpdates before destroying the position.
    • asyncUpdate() now defensively skips stale pids that are no longer present in self.positions.
  • Added regression coverage:
    • New test: cadence/tests/close_position_async_queue_stale_test.cdc
    • New helper transaction: cadence/tests/transactions/flow-alp/pool-management/async_update_all.cdc

Tests

  • Added regression test:
    • test_closePosition_clearsQueuedAsyncUpdateEntry
  • Ran:
    • flow test cadence/tests/close_position_async_queue_stale_test.cdc
    • flow test cadence/tests/async_update_position_test.cdc
    • flow test cadence/tests/close_position_with_queued_deposits_test.cdc

@liobrasil liobrasil requested a review from a team as a code owner March 4, 2026 22:21
@liobrasil
Copy link
Contributor Author

Performance note on queue cleanup:

Current fix is safe and linear, but closePosition still does queue work. If we want stricter gas bounds on mainnet, we should avoid any queue scan from closePosition.

Two paths:

  1. Minimal change (bounded close path):
  • Remove eager queue cleanup from closePosition.
  • Keep only stale-skip logic in asyncUpdate() when a queued pid no longer exists.
  • Benefit: closePosition becomes constant-time w.r.t. queue length.
  1. Best-performance pattern (recommended):
  • Keep positionsNeedingUpdates as append-only queue.
  • Add inQueue: {UInt64: Bool} and a queue head index (no removeFirst).
  • Enqueue: append only if inQueue[pid] != true.
  • Close: mark pid inactive / clear inQueue in O(1), no scan.
  • Async callback: read by head index, skip stale/inactive pids, advance head.
  • Optional periodic compaction to reclaim storage.

Option 2 gives predictable O(1)-style enqueue/close behavior and avoids costly array shifts/scans under load.

@liobrasil liobrasil mentioned this pull request Mar 4, 2026
6 tasks
@nialexsan nialexsan merged commit 047a6a7 into nialexsan/close-position Mar 4, 2026
@nialexsan nialexsan deleted the lionel/fix-close-position-stale-async-queue branch March 4, 2026 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants