Treat an unobserved order outcome as unknown instead of failed - #4405
Merged
Conversation
A liquidity order whose request left our side without an observed answer was recorded as Failed. That asserts knowledge we do not have, and it is not inert: a failed pipeline pauses its rule, and the rule auto-reactivates after reactivationTime, so the same request is issued again a few minutes later. Two Scrypt withdrawals hit exactly this path and were mis-recorded. Both requests timed out waiting for the BalanceTransaction update; both were stored as Failed with no correlationId, and both had in fact executed at the venue - the exchange transaction and the balance drop match the order amount to the cent. No double withdrawal resulted, but only because the balance had already fallen by the time the rule retried. That balance is served from a push cache with no freshness guarantee, so the safeguard cannot be relied on in the very situation that produces the timeout. Four changes, following the payout subdomain, which already solves this problem for blockchain broadcasts: 1. Reserve the venue reference before sending. Scrypt is the one integration that lets us choose it (ClOrdID / ClReqID), but it was generated inside the service and only returned on success, so a timeout lost it for good and left nothing to look up. Integrations may now supply a reference through reserveCorrelationId, which the pipeline persists before the request goes out; the id is derived from the order id, satisfying the venue's daily uniqueness requirement without a random component. 2. Add LiquidityManagementOrderStatus.UNCERTAIN, the counterpart to PAYOUT_UNCERTAIN. It is terminal for the pipeline - checkRunningPipelines already leaves such an order alone, so no rule is paused and nothing auto-reactivates - but not for the order. 3. Classify the send boundary fail-closed. Request timeouts now carry their own error type rather than a message string, so they can be told apart from a dropped socket, which proves the request never completed. Only the latter stays an ordinary failure. As a consequence idempotent reads - fetch and fetchAll, which cannot affect venue state - retry on timeout instead of ending the whole order; that alone covers 47 of the 49 timeouts observed over two weeks. 4. Reconcile instead of repeat. resolveUncertainOrders asks the venue what happened and never re-sends. Absence only counts as proof after a grace window, an unreachable venue leaves the order in quarantine, and reconciliation runs before any new order is issued. Observability, so a quarantined order is not silently parked: uncertainLmOrderCount is exposed on the liquidity observer, the quarantine mail is pinned per order and debounced, and the pipeline failure mail is pinned per rule and debounced - one incident used to send a mail per retry. The success mail is dropped; it accounted for 211 of 255 liquidity mails in a week and carried no information, which is what made the mails that matter unreadable. Also fixes an unrelated spin: an error outside the three known exception types left the order in Created, and startNewOrders reported a change regardless, so the caller's while loop could not terminate.
Three gaps in the first pass, all of the same shape as the bug it fixes. The amend boundary was still open. checkTrade may cancel-replace or restart an order from inside the completion check, and both created a fresh reference that was never recorded, so a replacement whose confirmation never arrived could not be looked up even in principle - and the error fell through to OrderFailedException, which pauses the rule and reissues the trade. Callers now supply the replacement reference, derived from the order row as <prefix><orderId>-<referencesUsed>, so it is reproducible without an extra column. An unconfirmed outcome there quarantines the order, and reconciliation enumerates the replacement candidates rather than only the current reference. "Connection closed" was treated as proof that nothing was sent. It is not: requestWithId hands the payload to the socket before registering the pending entry, and a later close rejects that entry with a generic message that says nothing about whether the venue acted. The classification is now fail-closed - only an explicit rejection from the venue, which proves the request was seen and refused, keeps an error an ordinary failure; everything else is an unknown outcome. Over-classifying is self-correcting, because an error that truly happened before the send leaves no trace at the venue and reconciliation settles the order as failed after the grace window. hasPendingOrders did not count quarantined orders. That gate is what stops a second rule on the same exchange from acting while funds are unaccounted for, so an unresolved order has to block there. Same reasoning applied to getProcessingOrders, and to getPendingTx, where omitting the status would have dropped the amount in question out of the financial log. Alert debouncing was keyed by rule alone. Suppression compares only the key, never the body, so a genuinely different failure of the same rule was swallowed within the window. The key now includes a digit-insensitive hash of the cause: true repeats still collapse, a new cause still reports. Keying by pipeline instead would have restored the flood, since every retry is a new pipeline. Tests: the amend and reconciliation paths, quarantine on socket close, a venue rejection staying an ordinary failure, replacement-reference derivation, a pipeline left untouched while its last order is quarantined, and the timeout type and read retry in the connection itself.
Follow-up to the previous commit, from a second review pass. The amend fix did not fire. checkTrade's inner catch swallowed every error that was not TradeChangedException, attempted a best-effort cancel and returned false, so a timeout or a dropped socket during editOrder never reached the classification added for it - the order kept polling the old reference while a replacement created under the reserved one stayed invisible, and reconciliation never ran because the order never entered quarantine. Unless the venue explicitly rejected the amend, that path now raises a dedicated unconfirmed-write error, and the same guard wraps the restart in the cancelled branch. That error type is what the check path was missing. Message matching cannot separate a dropped socket on a read from one on a write, yet the first is harmless and the second is unresolved; the completion check now tests for the write boundary before the transient-error branch, which previously classified a connection drop during a restart as retry-next-tick. The rejection markers move next to the error types so the send path and the check path share one definition, extended with the edit rejection. The custom-asset balance sum was left out of the previous pass on the grounds that Scrypt is not among its systems. That reasoning was wrong: the generic quarantine in startNewOrders is not system-specific, so a Kraken or Binance order can reach the status too, and neither adapter can resolve it - its amount would silently drop out of the balance for as long as it stayed there. Tests cover the amend boundary end to end: an unconfirmed write surfaces as an unknown outcome, carries the replacement reference into the quarantine reason, and a plain dropped connection on the read path still resolves as retry-next-tick. The PR description has been rewritten to the final state; it still described the first iteration and, on the dropped-socket classification, asserted the opposite of what the code now does.
…owed The previous commit made an unconfirmed amend propagate out of checkTrade, but the guard was only exercised through the adapter. The defect lived one layer below, in the service, and the service had no checkTrade tests at all - which is why the first attempt at this fix looked right and did nothing. Three tests at that level: an unconfirmed amend leaves checkTrade as an unconfirmed-write error instead of being cancelled away and reported as "not complete"; the raised error carries the reserved replacement reference, so the order can be reconciled against it; and an amend the venue explicitly rejected still falls back to cancel-and-continue, since a rejection proves nothing was created.
A cross-vendor review of the previous state found four more spots where the code still concluded something it had not seen. Each is the same mistake the PR set out to remove. A CREATED order that already carries a reserved reference is no longer re-sent. That combination can only arise when a previous pass reached the send boundary and died before recording the result, so sending again is precisely the duplicate we are trying to avoid; it now goes straight into quarantine. The catch-all no longer quarantines everything. Whether a reference was reserved tells us whether the send boundary was crossed at all: without one, nothing can have been transmitted and an ordinary failure is correct. Quarantining those stranded configuration and factory errors in a state only a human can clear - and, since reconciliation needs the same integration that just failed to load, they could never clear themselves. Reconciliation checked the oldest reference first. A replaced order usually still exists at the venue in a cancelled state, so the original matched, the order was reported as sent, and the live replacement stayed untracked while the completion check polled a dead reference. Candidates are now tried newest first. Absence is no longer treated as proof of non-arrival. The venue offers no terminal "this reference was never accepted" reply, so a missing record after any amount of time is not evidence; concluding otherwise is what would let a rule reissue a request that later materialises. Such an order stays quarantined for a human, and its rule stays blocked. The grace window existed only to make that inference safer and is gone with it. Two more, from the same principle. A failure to READ an order the venue has acknowledged no longer fails it - that would release the rule to open a second position against the same funds - but is retried, with a dedicated type for "acknowledged, then vanished" that quarantines instead of failing. And the quarantined status is taken back out of the financial log's pending set: the log adds a pending amount back to the balance and nets it against the venue's locked funds, but an unsent order locks nothing, so counting it would inflate equity by its full amount - the one error direction that can hide a real loss from the safety threshold.
A follow-up review found four more places where the code still had to guess.
Rejections were recognised by message text, and one real terminal path phrased its message
differently ("Order <id> has been rejected"). It matched nothing, so after the previous commit
made unmatched errors retry instead of fail, a genuinely rejected order would have been retried
forever. Rejections now carry their own type, used by every path that turns a venue refusal into
an exception: it cannot be missed by rephrasing, and a transport error that happens to quote the
phrase can no longer masquerade as a settled outcome.
Reconciliation recovered a withdrawal from the venue's history but never cached it, while
getWithdrawalStatus reads only the cache. An order leaving quarantine on the strength of that
lookup would have polled a reference the cache still did not know, and never completed. The
match is now fed back through the same terminal-aware cache write as a live push.
A bare request timeout in the completion check can only come from a read, because every write
there is already wrapped. Quarantining it stranded an order that nothing at the venue had
touched, so it is retried like any other read problem.
And the gap left by the previous commit: with absence no longer proving non-arrival, nothing
released a genuinely unsent order, so its rule would stay blocked indefinitely with no operator
action available. There is now a guarded admin endpoint, modelled on the payout subdomain's
retry guard - the caller must assert that the venue was checked and name where, and the
assertion is recorded on the order.
Also corrected: a comment claimed the socket send precedes registering the pending request,
where the code does the reverse; the conclusion it supported is unaffected, since what makes a
close ambiguous is that the bytes may already be on the wire. Stale grace-window wording removed
from code and description, and the description now matches the final behaviour, including the
deliberate exclusion of the quarantined status from the financial log's pending set.
Seventh pass. Six findings taken, one declined. A refused amend now reports itself. checkTrade swallowed the refusal and returned "not complete", so the caller never learned that the replacement reference was spent - the venue requires them to be unique, so the next tick derived the very same one and the pair could loop. The refusal carries the spent reference, the order records it, and the derivation moves on while tracking stays on the original, which a refused amend leaves live. Reconciliation no longer adopts a rejected replacement. Any returned record counted as proof of sending, so a replacement the venue had refused would be adopted, immediately fail the order, and release the rule although the original was still standing. Both resolution paths now write conditionally. Automatic reconciliation and an operator can hold the same order at once, and an unconditional save let whoever finished last win - including failing an order the venue had just confirmed as live. The status is part of the WHERE clause and a lost race is skipped, or reported as a conflict on the manual path. A venue error reply is no longer a plain error. It carries its own type, deliberately distinct from a rejection: "unknown reqid" arrives the same way and means the venue lost our request context, which for a mutation is as open as silence. A cached withdrawal record that is not terminal no longer short-circuits the history lookup - the missing terminal push is exactly what such a record would be hiding. The manual endpoint was too thin: the asserted flag and the caller were dropped at the edge, a whitespace-only reference passed validation, and the log line preceded the write. The service now re-asserts the claim, normalises and caps the reference, records who authorised it, persists before logging, and returns nothing rather than the entity. DECLINED: extending this to the ccxt venues. They share the gap, but they have no client order id today and no reconciliation lookup, so propagating the unknown outcome would quarantine orders across every exchange with no exit but a human - a worse position than today, on venues where nothing has gone wrong. Recorded in the description as its own piece of work.
Conformity pass. The manual-resolution reference was unbounded while both analogous DTOs in the repository cap it at 1024 characters, and it was written to the log untruncated; the cap now lives in the DTO, so the service records the validated value instead of silently shortening it. Two update mocks in the tests dropped their types for no reason and are typed again. On the contract question: the new order status appears in no DTO, and both endpoints that expose these orders are admin-guarded and excluded from the published surface, so there is no public contract change. Whether the internal admin view enumerates statuses exhaustively cannot be determined from this repository, so the description asks the reviewer to confirm it rather than leaving the question unasked.
The compare-and-set added earlier decides who writes first, and first is not the same as right. If an operator releases a quarantined order in the same moment reconciliation confirms the request exists at the venue, the operator's write lands, the order is marked failed, the pipeline releases and the rule can issue a second order against a live position. The manual path now asks the venue itself before releasing, and refuses when the reference is found - a positive observation outranks the operator's judgement, and the order is left for the next reconciliation pass to move on. A lookup that cannot be performed does not block the release: the operator has asserted an independent check, and a reference the venue can no longer be asked about must not turn into an order nobody can ever clear.
… unobservable case Three findings from the sixth pass, all of them consequences of the previous round's fixes. A replacement reference was only recorded once it was accepted or explicitly refused. In between it was neither the current reference nor a spent one, so a replacement whose confirmation was lost could be derived a second time. The reference is now claimed and persisted immediately before the request goes out, for both the amend and the restart. Reconciliation could still fall through to a superseded predecessor. An accepted replacement may simply not be visible at the venue yet, while the order it replaced still is; matching the older one reported the request as sent and left the live replacement untracked. An older reference is now considered only after the newer one was explicitly rejected - anything else keeps the order quarantined. A positive observation on the manual path was refused but not recorded. The row stayed quarantined, so a later attempt made while the venue happened to be unreachable could still release it and undo what had been seen. The observation is now persisted first, which takes the order out of manual reach entirely. And an acknowledged order that cannot be observed no longer polls forever. It was kept deliberately - failing it would release the rule against a live position - but the manual path only accepts quarantined orders, so there was no way out at all. Past the same age at which the venue itself is considered to have lost an order, it is quarantined: still not declared failed, but now reachable for a human.
The seventh pass found that the previous commit had broken the very case this work exists for. Reconciliation built its candidate list by deriving the NEXT reference and checking that first. For a freshly quarantined order that reference has never been sent, so it was absent — and since the previous commit made an absent newest reference stop the search, the reference that had actually gone out was never looked at. Every quarantined order would have stayed quarantined until somebody released it by hand. The tests hid it by answering every queried reference. Reconciliation now walks exactly the references this order has put on the wire, newest first, ordered by attempt number rather than by how the list happened to be stored. Nothing is synthesised. Two more from the same pass. The age bound that stops an unobservable order from polling forever sat behind the transient-transport branch, so an old order whose socket kept dropping never reached it — a dropped socket that keeps dropping is, from here, indistinguishable from one that will never answer, so the bound now applies to both. And a positive observation could still lose the write race: if somebody released the order as not executed while reconciliation was watching the venue confirm it, the release landed and the observation was discarded. An observation outranks a judgement, so it is now taken back — the order returns to in progress, with an alert, because whoever released it needs to know their check missed something.
Five findings from the eighth pass. The reclaim introduced last time was too broad. It matched any failed row by id, so an order that had ended for an entirely unrelated reason could be resurrected by a late positive observation. A not-sent resolution now stamps its failures, and only a failure carrying that stamp can be taken back — the reclaim exists to overrule a judgement, never the venue. The manual path ignored whether its own positive observation actually landed. If a negative resolution won the write, the observation was discarded; it now reclaims in exactly the same way as automatic reconciliation. Three paths could wait forever. An acknowledged withdrawal whose terminal update is never seen, an order left in a pending status whose update is missed, and - already bounded for trades - the unobservable case, all answered "not complete" indefinitely while the manual path accepts only quarantined orders, so there was no way out at all. All three now fall under the same age bound and become unknown outcomes rather than failures. And reconciliation can conclude a negative again. When every reference an order ever put on the wire comes back rejected, nothing was created — unlike mere absence that is a real negative, so the order is resolved instead of being asked about forever while its rule stays blocked.
❌ ESLint: 4 errors, 1 warnings |
The previous commit added a second case block for the pending statuses instead of extending the one already there, which is a duplicate-case lint error. The bound now lives in the existing branch, and the sixty minutes it shares with the "order cannot be found" path is a named constant rather than a literal repeated in two places. This should have been caught before pushing: the lint run had already reported it and the commit went out anyway.
The ninth pass found the two drifting together again. The previous commit quarantined an order that had been pending too long, and a withdrawal the venue knew about but had not settled. Both are OBSERVATIONS - we know exactly where the order stands - so quarantine was the wrong shelf: reconciliation found the reference, handed the order straight back as sent, and the next completion check quarantined it again. Every transition reported a change, so the pipeline loop kept going round, querying the venue each time, while manual release refused because the reference existed. Quarantine now means only what it says: we cannot tell whether the request took effect. A pending order simply waits, however old it is, and a withdrawal the venue has a record for waits too. Only the genuine blind spot - no record at all, past the age at which the venue is considered to have lost an order - is still quarantined. An order that waits too long is a stuck order, which the monitoring counter already surfaces; it is not an unresolved one. The same pass found no route to an automatic duplicate execution across the initial send, amend, restart, rejection, reference-claim and reclaim paths.
…eal blind spot Two findings from the tenth pass, plus three conformity points. A replacement the venue had accepted could be lost. The claim persists the new reference before sending, but adopting it afterwards is an ordinary save - and if that save failed, the row still named the predecessor while the venue had already cancelled it. The next check restarted from that predecessor and placed a second order alongside the live replacement. Before anything may write again, a claimed reference the venue is working is now adopted; a rejected one is skipped. The age bound was too broad. It fired for any failure after the check began, so an order the venue could still show us was quarantined because pricing or aggregation had failed - and reconciliation, finding the reference, handed it straight back for the next check to quarantine again. The bound now applies only when the order itself cannot be observed. From the conformity pass: the route follows the repository's camelCase majority (70 to 32); the audit reference is trimmed before validation, as eleven other string DTOs do; and the new tests use typed venue fixtures instead of widened literals. Not changed, with reason: the demand to remove every `any` from tests contradicts the verified house convention - the pattern appears 1441 times across this repository's specs. The cases where typing was straightforward are typed; the convention is not overturned in passing.
Three sequences found in review could still end in a second request against the same funds. A predecessor is not a replacement. After an amend this row DID record, the superseded original sits in the reference list as cancelled — and adoption, which only skipped the current reference, would take it back and restart the very quantity the replacement was working. Adoption now considers references newer than the current one only. A claimed reference the venue does not show, or cannot be asked about, is not a refusal. It is recorded before the request leaves, so it may be live at the venue right now; the check may not step past it and carry on with the predecessor. Such a claim now blocks every write on the order, and the wait ends the same way any other blind spot does: past the age at which the venue itself is considered to have lost an order, it goes to a human. Applying a positive observation takes two writes — release the quarantine, or take back a negative resolution that got there first. A crash or a failed write in between left the order in a state nothing ever read again, while its rule was free to plan anew. Failures stamped as not-sent are now re-examined alongside the quarantined ones for an hour, so the second write is simply retried. Six of the seven added tests fail without the corresponding change; the seventh pins the existing two-step reclaim.
… release An observation that the venue does know the order becomes durable in exactly one write. If that write does not land — the process ends, the statement fails — the observation is gone, and the order sits failed while the venue is working it. The hour-long window that covered this was itself a way to lose it, so there is no window any more: a failure stamped as not-sent stays eligible for reconciliation without an age limit. What that would otherwise cost is a venue lookup per settled failure on every tick, for the rest of its life. So each such order is asked about once per process instead. That is the same coverage — the observation can only be lost by this process ending or its write failing, and neither survives into the next one — without the standing load. The release endpoint updates an order, so it is a PUT, per the REST section of CONTRIBUTING. One test also went back to the typed venue-order helper instead of casting a literal.
Two review passes chased the same thing from opposite sides: re-examining terminal failures kept a lost observation recoverable, but it also meant a leading-wildcard scan over every historical failed row on each pipeline run, and the memo that bounded that cost treated an inconclusive lookup as settled — so a delayed confirmation was suppressed for the life of the process. Both go away with the mechanism. Reconciliation is back to quarantined orders only. The reclaim stays: it is one conditional statement and it still catches a negative resolution that lands mid-pass. What was missing was never the scan, it was the admission of failure. When the venue has confirmed an order and neither release matches — or the write throws — that observation is the one fact worth having, and it was being dropped as a log line. It now raises an alert naming the order and the reference, telling whoever reads it to treat the order as live at the venue. Consistent with the rest of this change: where we cannot be sure, a person decides, and no rule plans against those funds in the meantime. A release someone else performed correctly is recognised and stays quiet.
An alert is read at human speed; a rule reactivates in minutes. So reporting that the venue confirms an order — while the row stays failed — still leaves the scheduler free to plan against funds that are already committed. Reporting was the wrong half of the answer. Anything short of a clean release now puts the order back into quarantine, which is the state this subdomain already treats as in flight with an open outcome: no rule plans against it, and the next reconciliation pass simply tries again. The alert stays, but on top of a state that holds by itself. The manual release had the same shape and now takes the same path: its refusal no longer depends on a write it never checked. Its message says which of the two happened. The report also no longer depends on a second read succeeding — that read is only what lets a genuinely safe outcome stay quiet, and a state that cannot be read is not one of them.
…ow already says A not-sent failure is eligible for reconciliation again, so an observation whose writes never landed — including the re-quarantine that normally catches that — is still applied afterwards. The bound on it is the venue's own reach: Scrypt's execution history goes back thirty days, and past that nobody can make an observation to apply. That is deliberately not a deadline for applying one, which is how the previous attempt could lose it; it is also what keeps the query off the entire failure history. Second: overruling a resolution was erasing the record of it. Both write-backs built their message from the copy of the row this pass started with, which predates the resolution being overruled — so a reclaim replaced the account that released the order and the reference they checked with a stale string. Both now read the row and append to what it actually says, and where it cannot be read, only the status changes.
Finding the failures that still owe reconciliation a second look was done with a wildcard match over every failure ever recorded, on a job that runs every ten seconds, and the attempts to bound that cost kept trading one hole for another: an in-memory memo lost the obligation on restart, a time window turned into a deadline for applying an observation. The obligation now lives on the row. A not-sent resolution stamps notSentResolvedAt; the first reconciliation pass that looks at the order again clears it. That is the coverage that was wanted — the pass writing such a resolution may be racing one that has just watched the venue confirm the same order, and an observation that could not be written would otherwise be gone — and it survives a restart, because it is a column and not a set in memory. Cleared after that one look, so settled failures are not put to the venue every ten seconds for good. Indexed, so finding them is an equality predicate. Two smaller things, both about not destroying evidence: The reclaim and the re-quarantine now guard their write on the exact reason they just read. Another resolution can land in between, and appending to the older copy would erase the newer operator and reference. And the verification reference is trimmed at the edges only. Util.trimAll removes every space — it is for identifiers like IBANs — and it was turning "venue console, ticket OPS-42" into one run of characters. That field is evidence somebody has to read. The migration is additive and nullable, with no backfill. It was not run against a Postgres instance: this machine has none.
…ent in the reason Clearing the marker was guarded by order and status alone, so a pass that had been looking at one resolution could clear the marker of a newer one written in the meantime — dropping the further look that newer resolution was owed, which is the whole point of the marker. The clear is now conditional on the exact marker the pass started from. A marked order whose integration has no lookup at all could never have its recheck happen, so the marker stood forever and every pass selected the row only to skip it. Such a row is released instead. And the column is named for what it is: work still owed, not a record of when the resolution happened. That moment now goes into the order's own reason, which nothing clears — so releasing the marker cannot make it unrecoverable.
The rename to notSentRecheckDue added the new migration but left the old one in place, so both would have run and the table would have gained two columns instead of one. Only the new file belongs here; neither has been merged, so there is nothing deployed to correct.
A venue that cannot be reached was reported the same way as a venue that answered and had no record. Both arrived as UNRESOLVED, so the recheck an order still owed was retired on the strength of a lookup that never happened — and with it the observation it was being kept for. A failed lookup is now UNAVAILABLE: no question reached the venue, so nothing was looked at, and the order stays marked until something actually is. Separately, an order can outlive the adapter that made it. The integration factory returns null for a system or command that is no longer registered, and both the reconciliation loop and the manual release dereferenced it: the loop would have thrown on every pass forever, and the manual release refused an order that nothing could ever look up again — leaving it quarantined with no way out. Both handle the absence now, and a marked failure nobody can ask about is released rather than selected and skipped for good.
A release said the request never left, and the order became a plain failure on the spot. That is terminal: the pipeline finishes, the rule reactivates, and it may issue the request again — all while a confirmation that the order IS live could still have been in flight and simply lost its race to the write. Keeping such an order reconcilable afterwards did not help, because between the release and any later correction the rule was already free. So the release no longer ends the order. It is recorded, the order stays quarantined, and reconciliation puts it into effect on the next pass — normally seconds later — once the venue has answered that it has no record either. Two negatives, one of them from a person who looked. A venue that cannot be reached answers nothing, so nothing happens and the order keeps blocking; a venue that confirms the order overrules the release outright. An all-references-refused verdict still fails the order directly. That is the venue's own answer, not a judgement, and it needs no confirming. This removes the reclaim path entirely: nothing writes a failure behind a pending release any more, so there is no negative resolution left to take back.
Ending an order is the one step here that cannot be taken back, so it now compares and sets on the exact pending release the pass looked at. A release written since has a confirmation of its own outstanding, and completing the older reading would skip it. The one case where a release does not wait for the venue — an order no integration can look up any more, where no answer can ever come — is now stated in the same places the rule is: the column, the DTO, the migration and the pull request text. It was only in the code. The release tests asserted against the in-memory entity, which the reconciliation loop would have left looking correct even if nothing had been stored. They now assert the conditional write itself, its predicate and its payload, and that the pass reports a change. One comment still described the reclaim path that was removed.
When the venue confirms an order and the repair that holds it blocking cannot be written, the failure was swallowed. The order stayed a plain failure — nothing selects one of those again — while the venue worked it, and after the rule reactivated the request could go out a second time. The write is kept and repeated at the start of each pass, ahead of any new lookup. It asks the venue nothing; it only repeats what is already known, and it stops as soon as it lands or another path has put the order somewhere safe. Held in memory deliberately: the alternative is a durable queue nothing ever drains, and the one case this cannot cover — the process ending first — is covered by the alert that goes out at the same moment, which names the order and tells whoever reads it to treat it as live. Also: the branch that completes a release for an order nothing can look up now reports the change like every other branch, so the caller's loop sees it.
…until it is Two ways a confirmed venue order could still end up terminal. The first: applying an observation is a substantial write, and everything about it can fail. So the very first thing now is the narrowest write in the file — one column on a row that is still quarantined, cancelling any pending release. Once that lands, no judgement can end the order, whatever fails afterwards and even if this process stops. It is also what makes a quarantined row genuinely safe to stop retrying on: quarantined WITH a release pending is one inconclusive lookup away from being ended, and that is exactly what the observation contradicts. The second: holding an unwritten observation in memory kept it alive, but the rest of the pass carried on regardless — starting pipelines, advancing them, issuing orders — on a picture in which an order says failed while the venue is working it. The pass now stops after reconciliation for as long as any observation is unrecorded, and the next one retries the write before asking the venue anything.
…he after an unconfirmed cancel The first write for a confirmed order only stripped a pending release of its power. If a release had already ended the order, that write matched nothing and the repair came later — so until it did, the only thing between a live venue order and a second request was this process staying alive. The same statement now also puts an ended order back into quarantine. Two columns, no appended text, nothing it depends on; the reason why still follows separately, and if that never lands the order is at least still blocking. Second: when an amend is refused, the fallback cancel is a write as well. Its failure was noted and dropped. Unconfirmed, that cancel may have taken effect at the venue while the cached report still shows the order open — and a non-terminal cached report is never replaced by a later fetch, so every check afterwards would keep waiting on a picture that cannot change, with the order stuck in progress and out of reach of both reconciliation and the manual path. The cached report is dropped instead, so the next lookup has to ask the venue, and the refusal says the cancel went unconfirmed.
A release waits for the venue to answer, which is right — but a venue that answers nothing at all would have held a verified order for good. The wait exists to catch a confirmation that is in flight right now; after an hour of silence there is none in flight, only an operator who checked and is being ignored. This is a liveness bound, not a safety one, and nothing is concluded from the silence: the person who released the order concluded it. Silence merely stops being a veto. Also: an import out of alphabetical order.
…everywhere Whether a pending release has waited out an unreachable venue is something the order knows about itself, so it is answered by the order, with the bound alongside it, and the service is left with the orchestration. And the promise that a release waits for the venue was written in six places, all of which still named only one way out of it. Both are now stated wherever the rule is: an order no integration can look up, and a venue that has answered nothing for long enough. Neither concludes anything from silence — silence stops being a veto on the person who checked, it never becomes evidence. The entity gains its own tests, including the boundary either side of the wait.
The same promise is written in several places and two of them still named only one way the wait can end. No behaviour change.
The new import went in at the top instead of in order, and the two around it were already out of order. No behaviour change.
Collaborator
Author
|
This took 27 review passes to reach a clean bill on both correctness and conformity. Recording what they Double execution, closed in layers. Early passes found that adoption of a claimed replacement could walk A release that ended an order too early. The largest change came late. Concluding that a request never Making a confirmed observation survive. Several passes circled the same question — where does "the venue Audit trail. Overruling a release was overwriting the account that made it and the reference they Smaller, still real. An unconfirmed cancel after a refused amend left a stale open order in the cache Two points were argued and deliberately not changed; both are described in the PR body. The first is a Verification: 12 suites, 178 tests. Every behavioural change is pinned by at least one test that fails |
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.
Why
A liquidity order whose request left our side without an observed answer was recorded as
Failed. That asserts knowledge we do not have — and it is not inert: a failed pipeline pauses its rule, the rule auto-reactivates afterreactivationTime, and the same request goes out again minutes later.Two Scrypt withdrawals took exactly this path and were mis-recorded:
Failed, nocorrelationIdFailed, nocorrelationIdBoth are confirmed twice over: an
exchange_txrow for the exact amount, and a balance drop matching the order amount to the cent.No double withdrawal resulted — but only because the balance had already fallen by the time the rule retried. That balance is served from a push cache with no freshness guarantee and the connection has no liveness check, so the safeguard is least reliable in precisely the situation that produces the timeout.
The rule this PR applies
Never conclude an outcome that was not observed. If we cannot prove a request did not take effect, do not repeat it — look it up.
This follows the payout subdomain, which already solves the same problem for blockchain broadcasts (
PayoutBroadcastException/PAYOUT_UNCERTAIN), rather than inventing a second mechanism.What
1. Reserve the venue reference before sending. Scrypt is the one integration that lets us choose it (
ClOrdID/ClReqID), but it was generated inside the service and only returned on success — a timeout lost it, leaving nothing to look up. Integrations may now supply one viareserveCorrelationId, persisted by the pipeline before the request goes out. Derived from the order id, so it meets the venue's daily-uniqueness rule without randomness and is reproducible from the row alone.This extends to the amend boundary:
checkTradecan cancel-replace or restart an order from inside the completion check, and each of those creates a new venue order. Callers now pass a replacement reference derived as<prefix><orderId>-<referencesUsed>, so a replacement whose confirmation never arrives is still findable. Reconciliation enumerates those candidates, not just the current reference.2.
LiquidityManagementOrderStatus.UNCERTAIN. Terminal for the pipeline —checkRunningPipelinesalready leaves such an order alone, so no rule is paused and nothing auto-reactivates — but not for the order.3. Classify the write boundary, fail-closed. Only an explicit rejection from the venue proves a request did not take effect; the venue replied, so the outcome is known. Everything else — including a dropped socket — is an unknown outcome. A close is not proof of non-delivery: once
ws.sendhas run the bytes may already be on the wire, and the close that follows rejects the pending request with a generic message that says nothing about whether the venue acted on them.A rejection is carried by its own error type rather than recognised by message text, so it cannot be missed by rephrasing a message, and a transport error that happens to quote the phrase cannot masquerade as a settled outcome.
Over-classifying costs an operator a look at the venue; under-classifying is what moved money without a record. Since absence at the venue is not proof either, such an order waits for a human rather than clearing itself — deliberately the expensive direction, because the cheap one is the dangerous one.
A separate error type marks writes whose outcome was never confirmed, so the check/amend path can tell them apart from read-safe transport errors — a distinction message matching cannot make, since the same dropped socket is harmless on a read and unresolved on a write.
As a consequence idempotent reads (
fetch/fetchAll, which cannot affect venue state) retry on timeout instead of ending the whole order — that alone covers 47 of the 49 timeouts observed over two weeks.4. Reconcile instead of repeat.
resolveUncertainOrdersasks the venue what happened and never re-sends — and it can only ever confirm the positive. Scrypt has no terminal "this reference was never accepted" reply, so absence is not evidence at any age: such an order stays quarantined and its rule stays blocked. Reconciliation runs before any new order is issued, and tries the newest reference first, because a replaced order usually lingers at the venue in a cancelled state and would otherwise mask the live replacement.Adoption only ever moves forwards: a reference newer than the one the row names may be adopted, a
superseded predecessor never — after an amend the original lingers at the venue as cancelled, and taking it
back would restart the very quantity the replacement is working. And a claimed reference the venue does not
show, or cannot be asked about, blocks every write on that order rather than being stepped past: the
reference is recorded before the request leaves, so it may be live at the venue at that moment.
A release is accepted, not executed. Concluding that a request never left is the one judgement nothing
here can verify from the outside, and it can be made at the very moment reconciliation is watching the venue
confirm that same order. So the release does not end the order: it is recorded, the order stays quarantined,
and reconciliation puts it into effect on the next pass — seconds later — once the venue has answered that it
has no record either. Two negatives, one of them from a person who looked. A venue that cannot be reached
answers nothing, so the order keeps blocking; a venue that confirms the order overrules the release outright.
An all-references-refused verdict still fails the order directly: that is the venue's own answer, not a
judgement.
Two exceptions, both about liveness rather than safety: an order no integration can look up any more never
gets an answer, and a venue that has been unreachable for an hour is not going to give one. In both the
release takes effect on the operator's assertion, which is exactly what it was checked for — silence stops
being a veto, it never becomes evidence.
Where a confirmed observation cannot be written back at all, the order goes back into quarantine rather
than being merely reported — an alert is acted on at human speed while a rule reactivates in minutes — and
the alert goes out on top of that state, naming the order and the reference.
Since nothing releases a genuinely unsent order automatically, there is a guarded manual path (
PUT liquidityManagement/order/:id/resolveUncertain, admin only). It mirrors the payout subdomain's retry guard: the caller must assert that the venue was checked and name where, and the assertion is written onto the order — where it stays, since nothing overwrites it.Observing is not deciding
A failure to read an order the venue has acknowledged no longer ends it — that would release the rule to open a second position against the same funds. Such a check is retried, with a dedicated type for "acknowledged, then vanished" that quarantines instead of failing.
Status filters
UNCERTAINcounts as pending wherever an in-flight order matters, so an unresolved order cannot be stepped over:hasPendingOrders— the gate that stops a second rule on the same exchange from acting while funds are unaccounted for.getProcessingOrders— unfinished business belongs in the operator view.getPendingTx, which feeds the financial log, deliberately does not include it. The log adds a pending amount back to the balance and nets it against the venue's locked funds; a quarantined order may have locked nothing, so counting it would inflate equity by its full amount — the one error direction that can hide a real loss from the safety threshold.Observability
So a quarantined order is not silently parked:
uncertainLmOrderCounton the liquidity observer — no age threshold, one such order is already worth surfacing.Incidental
An error outside the known exception types left the order in
CreatedwhilestartNewOrdersreported a change regardless, so the caller'swhileloop could not terminate. Such an error now settles the order — quarantined if a reference had already been reserved (the send boundary was crossed), an ordinary failure if not, since nothing can have been transmitted.Notes for the reviewer
statusis avarchar, so the new value needs no schema change, and the reserved reference reuses the existingcorrelationIdcolumn. What does need one isnotSentRecheckDue(nullable, indexed, no backfill). It marks an order somebody has released as never sent, and while it stands the order stays quarantined. On the row rather than in memory, so a restart does not lose the pending release, and cleared once the venue has answered. It records work outstanding, not when the release was asked for: that moment goes into the order’s own reason, which nothing clears.ClOrdIDuniqueness but does not promise that a duplicate is rejected. That is exactly why the design reconciles rather than retries — the answer stops mattering.possible —
status: Uncertainplus clearing the pending release, matched againstUncertain,FailedorNotProcessable— so that from then on no judgement can end it and one that already had is repaired,whatever fails afterwards. And while any confirmed observation remains unrecorded, the pipeline pass stops
after reconciliation rather than starting or advancing anything on a picture where an order says failed
while the venue works it.
same moment — the cron lock is per process, as it is everywhere else in this codebase. If one instance is
told by the venue that the order does not exist while another is told it does, the first can complete a
pending release milliseconds before the second re-quarantines the order, and a pipeline advancing in that
gap could start its next action. Ending an order now compares and sets on the exact release examined,
which removes every stale-read variant of this; closing the last of it needs cross-instance serialisation
that no job in this subdomain has today, and inventing it here felt like the wrong place. Worth a look if
the deployment is ever multi-instance.
Consumer impact — checked
No public API contract changes.
LiquidityManagementOrderStatusappears in no DTO, and both endpoints thattouch these orders (
GET in-progressand the newPUT :id/resolveUncertain) areRoleGuard(UserRole.ADMIN)and
@ApiExcludeEndpoint(), so neither is part of a published surface.Uncertainis a new value on an existing field, so the question is whether any consumer enumerates thatfield exhaustively. None does: every consumer in this repository filters for
Complete(the accountingconsumer, the ledger cutover, the dashboard reconciliation) — and an order whose outcome is unknown must
not be booked, which is exactly what that filter already does. The service front end never reads this
subdomain at all. No sync is needed anywhere.
Verification
tsc --noEmit: cleanliquidity-management+exchangesuites: 12 suites, 178 tests, all passing