Skip to content

Commit 6035fe6

Browse files
committed
fix: handle BROADCAST_DEADLINE gracefully — deadlock fix, retry, configurable daemon attempts
The bot deadlocked after a BROADCAST_DEADLINE credential daemon response, leaving the grid unreconciled and the process hung. Three interrelated changes: ## Deadlock fix (fillLockAlreadyHeld:true) File: modules/dexbot_class.ts:4242 - Problem: _reconcileAfterUncertainBroadcast tried to acquire _fillProcessingLock, but it was already held by the fill-processing call chain (fill loop → safe-rebalance → COW broadcast → catch). AsyncLock is not reentrant, so the second acquire() queued forever. - Impact: reconcile never ran, no log output after the error. - Fix: pass fillLockAlreadyHeld:true from the catch block — the lock is always held at this call site. ## Bot-level retry for BroadcastUncertainError File: modules/dexbot_class.ts:2943-2972 - The daemon's broadcastWithRetry runs up to N attempts against a single 25s inner deadline (CREDENTIAL_DAEMON_INNER_DEADLINE_MS). If all expire, the error reaches the bot immediately. - Fix: new _executeWithRetryOnUncertain wrapper retries the broadcast once with a fresh 25s window before falling through to reconcile. - Skips retry when err.partialOnChainState is true (pair-mode grouped execution) — re-broadcasting would duplicate already-committed creates. ## Configurable daemon broadcast retries File: modules/constants.ts:271, credential-daemon.ts:407,421 - Hardcoded `attempt <= 2` replaced by CREDENTIAL_DAEMON_BROADCAST_RETRIES (default 3). - Daemon reads constant via TIMING lookup with ??2 fallback. ## Test coverage File: tests/test_uncertain_broadcast.ts - UNC-012: drives _updateOrdersOnChainBatchCOW through the BroadcastUncertainError path, asserts reconcile receives fillLockAlreadyHeld:true (deadlock regression guard). - UNC-013: asserts _executeWithRetryOnUncertain retries exactly once then re-throws on second BroadcastUncertainError. - UNC-014: asserts retry is skipped when partialOnChainState is set (pair-mode double-publish guard). - Also added missing stubs (lockOrders, unlockOrders, etc.) to makeBot() to support the deeper method path. ## Risk/Edge-case Notes - Bot-level retry adds up to ~25s per retry. Current config = 1 retry, so worst-case broadcast stall is ~50s (2 × 25s) before reconcile. - partialOnChainState skip only matters for pair-mode (outside-in create groups). Non-pair batches (updates/cancels) always set it false, so they always get the retry. - Daemon's broadcastWithRetry now reads TIMING at call time, not module-load time — consistent with the existing pattern used by CREDENTIAL_DAEMON_INNER_DEADLINE_MS. ## Testing Notes - npm test (uncertain broadcast suite) — all 24 tests pass. - Also verified: cow_orchestration_fixes, cow_commit_guards, main_loop_sync_fill_rebalance.
1 parent 5a20dbd commit 6035fe6

4 files changed

Lines changed: 170 additions & 6 deletions

File tree

credential-daemon.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -404,8 +404,9 @@ async function broadcastWithRetry(accountName: any, privateKey: any, broadcastFn
404404
}, innerDeadlineMs);
405405
});
406406

407+
const maxRetries = TIMING?.CREDENTIAL_DAEMON_BROADCAST_RETRIES ?? 2;
407408
const work = (async () => {
408-
for (let attempt = 1; attempt <= 2; attempt++) {
409+
for (let attempt = 1; attempt <= maxRetries; attempt++) {
409410
try {
410411
if (_nativeChainClient.getStatus() !== 'connected') {
411412
_nativeChainClient.setNodes(_nativeNodeList.length > 0 ? _nativeNodeList : NODE_MANAGEMENT.DEFAULT_NODES);
@@ -417,7 +418,7 @@ async function broadcastWithRetry(accountName: any, privateKey: any, broadcastFn
417418
await client.initPromise;
418419
return await broadcastFn(client);
419420
} catch (err: any) {
420-
if (attempt === 2) throw err;
421+
if (attempt === maxRetries) throw err;
421422
debugLog(`Broadcast failed (attempt ${attempt}), reconnecting: ${err.message}`);
422423
try { _nativeChainClient.disconnect(); } catch (_) {}
423424
}

modules/constants.ts

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,13 @@ let TIMING = {
267267
// path handles whatever takes longer.
268268
CREDENTIAL_DAEMON_INNER_DEADLINE_MS: 25000,
269269

270+
// CREDENTIAL_DAEMON_BROADCAST_RETRIES: Number of broadcast attempts inside the
271+
// credential daemon's broadcastWithRetry. Each attempt connects, signs, and pushes
272+
// the transaction. Total wall time across all attempts is capped by
273+
// CREDENTIAL_DAEMON_INNER_DEADLINE_MS above. 2 is a pragmatic balance: try once,
274+
// retry once on disconnect, while leaving room for a slow block before the deadline.
275+
CREDENTIAL_DAEMON_BROADCAST_RETRIES: 3,
276+
270277
// SAFETY_NET_SYNC_TIMEOUT_MS: Cap on the post-reconnect safety-net sync
271278
// in dexbot_class.ts. Must stay below the 20s shutdown lock timeout so
272279
// it never holds _fillProcessingLock longer than the shutdown deadline.

modules/dexbot_class.ts

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2940,6 +2940,37 @@ class DEXBot {
29402940
return false;
29412941
}
29422942

2943+
/**
2944+
* Execute operations with retry on BroadcastUncertainError.
2945+
* The daemon already retries internally against a 25s deadline.
2946+
* If all expire, a fresh bot-level attempt buys a new 25s window.
2947+
*
2948+
* Skips retry when partialOnChainState is true (pair-mode grouped
2949+
* execution where earlier groups already committed). Re-broadcasting
2950+
* the full operations array would duplicate those creates on chain.
2951+
*/
2952+
async _executeWithRetryOnUncertain(operations, opContexts) {
2953+
const MAX_RETRIES = 1;
2954+
for (let attempt = 1; ; attempt++) {
2955+
try {
2956+
return await this._executeOperationsWithStrategy(operations, opContexts);
2957+
} catch (err) {
2958+
const isRetriable = err instanceof BroadcastUncertainError
2959+
&& !err.partialOnChainState
2960+
&& attempt <= MAX_RETRIES;
2961+
if (isRetriable) {
2962+
this.manager.logger.log(
2963+
`[COW] Broadcast uncertain (attempt ${attempt}/${MAX_RETRIES + 1}), retrying...`,
2964+
'warn'
2965+
);
2966+
await this._ensureCredentialDaemonWritable('COW batch retry');
2967+
continue;
2968+
}
2969+
throw err;
2970+
}
2971+
}
2972+
}
2973+
29432974
/**
29442975
* Execute blockchain operations with appropriate strategy (single batch or pair mode).
29452976
* @param {Array<import('./types').CreatedOperation>} operations - Array of operation objects
@@ -4077,7 +4108,7 @@ class DEXBot {
40774108
// Execute batch
40784109
this.manager.logger.log(`[COW] Broadcasting batch with ${operations.length} operations...`, 'info');
40794110
this._lastBroadcastHeartbeatAt = Date.now();
4080-
const execution = await this._executeOperationsWithStrategy(operations, opContexts);
4111+
const execution = await this._executeWithRetryOnUncertain(operations, opContexts);
40814112
const result = execution.result;
40824113
const executedContexts = execution.opContexts;
40834114

@@ -4211,7 +4242,7 @@ class DEXBot {
42114242
// throwing would re-enter the catch loop on the next attempt and
42124243
// potentially double-publish.
42134244
if (err instanceof BroadcastUncertainError) {
4214-
return await this._reconcileAfterUncertainBroadcast(err, opContexts);
4245+
return await this._reconcileAfterUncertainBroadcast(err, opContexts, { fillLockAlreadyHeld: true });
42154246
}
42164247

42174248
// Handle hard abort

tests/test_uncertain_broadcast.ts

Lines changed: 127 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -882,14 +882,136 @@ function makeBot() {
882882
},
883883
requestStructuralGridResync: async () => {},
884884
_lastUnmatchedChainOrders: [],
885-
_pendingBroadcasts: new Map()
885+
_pendingBroadcasts: new Map(),
886+
lockOrders: () => {},
887+
unlockOrders: () => {},
888+
startBroadcasting: () => {},
889+
stopBroadcasting: () => {},
890+
_setRebalanceState: () => {},
891+
pauseFundRecalc: () => {},
892+
resumeFundRecalc: async () => {},
893+
_clearWorkingGridRef: () => {},
894+
_throwOnIllegalState: false
886895
};
887896
bot.account = 'test-account';
888897
bot.privateKey = 'test-private-key';
889898
bot._currentCycleId = 1;
890899
return bot;
891900
}
892901

902+
async function testCowCatchBlockPassesFillLockAlreadyHeld() {
903+
console.log('\n[UNC-012] _updateOrdersOnChainBatchCOW passes fillLockAlreadyHeld:true to reconcile...');
904+
const bot = makeBot();
905+
const slotId = 'slot-unc-012';
906+
const actionOrderId = '1.7.999999';
907+
const origBuildUpdate = chainOrders.buildUpdateOrderOp;
908+
const origExecuteStrategy = bot._executeOperationsWithStrategy;
909+
const origReconcile = bot._reconcileAfterUncertainBroadcast;
910+
const origValidateFunds = bot._validateOperationFunds;
911+
let reconcileOptions = null;
912+
913+
bot.manager.orders.set(slotId, { id: slotId, type: 'sell', price: 0.05, size: 100, orderId: actionOrderId });
914+
bot.manager.getChainFundsSnapshot = () => ({});
915+
916+
chainOrders.buildUpdateOrderOp = async (account, orderId, delta, rawOnChain) => ({
917+
op: { fee: { amount: 0, asset_id: '1.3.0' }, op_data: {} },
918+
finalInts: { sell: 100, receive: 1 }
919+
});
920+
921+
bot._validateOperationFunds = () => ({ isValid: true, summary: '', violations: [] });
922+
923+
bot._executeOperationsWithStrategy = async (_ops, _ctxs) => {
924+
throw new BroadcastUncertainError('uncertain', { batchId: 'unc-012', timeoutMs: 30000 });
925+
};
926+
927+
bot._reconcileAfterUncertainBroadcast = async (_err, _ctxs, opts) => {
928+
reconcileOptions = opts;
929+
return { executed: false, uncertain: true };
930+
};
931+
932+
const cowResult = {
933+
workingGrid: {},
934+
workingIndexes: {},
935+
workingBoundary: {},
936+
actions: [{
937+
type: COW_ACTIONS.UPDATE,
938+
id: slotId,
939+
orderId: actionOrderId,
940+
newSize: 100,
941+
order: { type: 'sell', price: 0.05, size: 100 }
942+
}]
943+
};
944+
945+
try {
946+
await bot._updateOrdersOnChainBatchCOW(cowResult);
947+
assert(reconcileOptions, '_reconcileAfterUncertainBroadcast must be called');
948+
assert.strictEqual(reconcileOptions.fillLockAlreadyHeld, true,
949+
'reconcile must receive fillLockAlreadyHeld:true to avoid reentrant deadlock');
950+
} finally {
951+
chainOrders.buildUpdateOrderOp = origBuildUpdate;
952+
bot._executeOperationsWithStrategy = origExecuteStrategy;
953+
bot._reconcileAfterUncertainBroadcast = origReconcile;
954+
}
955+
console.log('✓ UNC-012 passed');
956+
}
957+
958+
async function testExecuteWithRetryOnUncertainRetriesOnce() {
959+
console.log('\n[UNC-013] _executeWithRetryOnUncertain retries once on BroadcastUncertainError then throws...');
960+
const bot = makeBot();
961+
const origExecuteStrategy = bot._executeOperationsWithStrategy;
962+
let callCount = 0;
963+
964+
bot._executeOperationsWithStrategy = async (_ops, _ctxs) => {
965+
callCount++;
966+
throw new BroadcastUncertainError('uncertain', { batchId: 'unc-013', timeoutMs: 30000 });
967+
};
968+
969+
try {
970+
await assert.rejects(
971+
() => bot._executeWithRetryOnUncertain([], []),
972+
(err) => {
973+
assert(err instanceof BroadcastUncertainError, 'must throw BroadcastUncertainError');
974+
assert.strictEqual(callCount, 2, 'must try exactly 2 times (1 initial + 1 retry)');
975+
return true;
976+
}
977+
);
978+
} finally {
979+
bot._executeOperationsWithStrategy = origExecuteStrategy;
980+
}
981+
console.log('✓ UNC-013 passed');
982+
}
983+
984+
async function testExecuteWithRetrySkipsPartialOnChainState() {
985+
console.log('\n[UNC-014] _executeWithRetryOnUncertain skips retry when partialOnChainState is set...');
986+
const bot = makeBot();
987+
const origExecuteStrategy = bot._executeOperationsWithStrategy;
988+
let callCount = 0;
989+
990+
bot._executeOperationsWithStrategy = async (_ops, _ctxs) => {
991+
callCount++;
992+
const err = new BroadcastUncertainError('uncertain', { batchId: 'unc-014', timeoutMs: 30000 });
993+
err.partialOnChainState = true;
994+
err.groupsBroadcast = 1;
995+
err.groupsTotal = 2;
996+
throw err;
997+
};
998+
999+
try {
1000+
await assert.rejects(
1001+
() => bot._executeWithRetryOnUncertain([], []),
1002+
(err) => {
1003+
assert(err instanceof BroadcastUncertainError, 'must throw BroadcastUncertainError');
1004+
assert.strictEqual(callCount, 1, 'must NOT retry when partialOnChainState is true');
1005+
assert.strictEqual(err.partialOnChainState, true);
1006+
return true;
1007+
}
1008+
);
1009+
} finally {
1010+
bot._executeOperationsWithStrategy = origExecuteStrategy;
1011+
}
1012+
console.log('✓ UNC-014 passed');
1013+
}
1014+
8931015
async function main() {
8941016
console.log('Running uncertain-broadcast recovery tests...');
8951017
await testFingerprintDeterministic();
@@ -915,8 +1037,11 @@ async function main() {
9151037
await testAutoCancelUsesSyncEngineChainOrderIdShape();
9161038
await testAutoCancelSkipsWhenPendingBroadcasts();
9171039
await testAutoCancelSkipsFingerprinted();
1040+
await testCowCatchBlockPassesFillLockAlreadyHeld();
1041+
await testExecuteWithRetryOnUncertainRetriesOnce();
1042+
await testExecuteWithRetrySkipsPartialOnChainState();
9181043
testsComplete = true;
919-
console.log('\nAll uncertain-broadcast tests passed.');
1044+
console.log('\nAll uncertain-broadcast tests passed (incl. retry + deadlock regression guards).');
9201045
}
9211046

9221047
main().catch((err) => {

0 commit comments

Comments
 (0)