fix(keeper): drop stale watches and idle duplicate rejections#242
Conversation
84ef2ba to
25226ee
Compare
25226ee to
ee87527
Compare
ee87527 to
5b05f8e
Compare
5b05f8e to
ef3f9d3
Compare
ef3f9d3 to
850f30c
Compare
850f30c to
cf6f224
Compare
cf34ce4 to
8dce752
Compare
826abef to
104c6ec
Compare
8dce752 to
3180972
Compare
3180972 to
5cbce6a
Compare
5cbce6a to
e8acf48
Compare
e8acf48 to
a9b18f5
Compare
a9b18f5 to
2763e33
Compare
2763e33 to
6cfedbe
Compare
c36397a to
5179a35
Compare
5179a35 to
25ad223
Compare
25ad223 to
c7af694
Compare
c7af694 to
6b646d4
Compare
6b646d4 to
1de11c3
Compare
lgahdl
left a comment
There was a problem hiding this comment.
The three fixes are correct and the tests are solid. A few items below.
| // documented branches from anomalies. NOTE: the "not mirrored" | ||
| // skip path was retired with hash-only submission; this | ||
| // classification is kept for historical report comparability | ||
| // until the harness is reworked around the observer-only |
There was a problem hiding this comment.
This guard still fires on real fixtures: any fixture whose app_data_resolved is None (collected before hash-only submission, or whose hash 404'd) returns RejectedExpected("app_data hash not mirrored") regardless of what the strategy actually did. A fixture that the strategy fails to submit for a completely unrelated reason (watch drop, gate, etc.) will be silently misclassified as expected-skip rather than landing in RejectedUnexpected for triage.
Either remove the guard (the RejectedUnexpected bucket is the right place for it now) or invert the condition to fx.app_data_resolved.is_some() so it only fires for fixtures that actually had resolved data — but the label still needs updating in that case.
| PollOutcome::DontTryAgain => watches.remove(watch)?, | ||
| PollOutcome::DontTryAgain => { | ||
| // The removal is permanent; leave a trace of it even | ||
| // for sources that do not log their own outcomes. |
There was a problem hiding this comment.
Permanent watch removal warrants warn, not info. The comment above correctly says sources may not log their own outcomes — for such a source the only signal that a watch vanished forever is this line, and at info level that's easy to miss in production logs.
| // for sources that do not log their own outcomes. | |
| tracing::warn!("{} dropped watch {}", source.label(), watch.key()); |
| } | ||
| Err(err) => { | ||
| // Success wearing an error status: the orderbook already | ||
| // holds this exact order. Record the receipt under the |
There was a problem hiding this comment.
This correctly writes to dedup_key (submitted:{uid_hex}), but the normal-submit success path a few lines above writes submitted:{server_uid} instead. If the server returns a UID that differs from the client-side uid_hex (the drift path that already logs a warn), the dedup guard reads dedup_key and will not fire next block — the module re-POSTs until validTo. The DuplicatedOrder fix here closes that window correctly, but the root asymmetry in the success path is now visible by contrast. The chassis submit_ready uses the client UID consistently on both paths.
| /// stops the submit path downstream ([`gpv2_to_order_data`] fails the | ||
| /// same way there); an unsupported chain id does not, so a caller | ||
| /// keying idempotency on this value alone re-submits until `validTo` | ||
| /// on such a chain - bounded, but callers adding new chains should |
There was a problem hiding this comment.
The advisory about chain-adders is buried mid-sentence in the None return-value description, making it hard to scan. Consider moving it to a # Note section so the None condition can be read on its own.
| /// on such a chain - bounded, but callers adding new chains should | |
| /// on such a chain — bounded, but bounded only by `validTo`. New chains | |
| /// should be registered in `cowprotocol::Chain` before go-live. |
d5ff2c3 to
ce55637
Compare
76c778d to
498c843
Compare
3b81e00 to
b58db49
Compare
b58db49 to
6ba85d4
Compare
…he order A constructor rejection (zero from, validTo beyond the client-side one-year horizon) is deterministic for the polled payload: skipping with the watch intact re-polled and re-warned on every block forever. Route it through the retry ledger as a drop, matching the pre-keeper net effect where the orderbook rejected the shipped body (e.g. ExcessiveValidTo) and the classifier dropped the watch. The unknown-enum-marker skip keeps its pinned keep-the-watch behaviour. Also log the keeper-level DontTryAgain removal so a permanent drop is observable even for sources that do not log their own outcomes.
classify_api_error maps DuplicatedOrder to TryNextBlock on the premise that the caller records the submitted: receipt - true for the run, but stop-loss consumed the classification without the compensating write, so a duplicate rejection re-POSTed every block until validTo. Mirror run::submit_ready: treat already-submitted as success wearing an error status, write the marker the dedup guard reads, and idle.
…yer dev cycle WatchSet::key never used the host parameter, forcing a meaningless turbofish at every call site (one test existed solely to prove two instantiations agree). keeper::watch_key is the free canon; WatchSet::key stays as a thin delegate for discoverability. The keeper acceptance tests only touch the local-store seam, so they now run against nexum-sdk-test's MockHost (the same MockLocalStore type), shrinking nexum-sdk's dev cycle from the cross-layer shepherd-sdk-test pair - which dragged the whole CoW domain layer into the world-neutral crate's dev graph - to the within-layer pair its own mock crate documents.
… path A revert that classifies to DontTryAgain destroyed the watch with only an Info outcome label; the selector and node message needed to triage a wrongly-dropped watch were discarded. Warn with both before returning the outcome, and pin the log lines in the drop test. The test-only watch_key wrapper now reuses the keeper free fn.
OrderCreation::from_signed_order_data was renamed to OrderCreation::new in cowprotocol 0.2; and only the unknown-marker case of order_uid_hex returning None also stops the submit path downstream - an unsupported chain id does not, so say so instead of claiming both do.
The epic removed resolve_app_data and the orderbook-mock's GET /api/v1/app_data route, but the replay harness still programmed that route against a strategy that never requests it. Drop the dead programming (and the now-unused shepherd-sdk dependency) and refresh the module doc to the hash-only submission flow. The RejectedExpected classification is kept for historical report comparability until the harness is reworked around the observer-only strategy (follow-up).
…atch-channel move The overview's SDK table predated the epic's headline surface: add the nexum-sdk keeper row and the shepherd-sdk cow::run row. Amend ADR-0004 with the move of the [patch.crates-io] channel from bleu/cow-rs to nullislabs/cow-rs and what the pinned rev carries.
Numeric issue/PR references in .rs files go stale across repository moves; describe the change instead of pointing at the review.
What
Fixes two watch-lifecycle bugs in the keeper's
cow::runpoll loop and adds permanent-drop observability:submit_ready(crates/shepherd-sdk/src/cow/run.rs): anOrderCreationconstructor rejection (validTobeyond the client-side one-year horizon, zerofrom) no longer skips-and-keeps the watch in an unbounded per-blocketh_call+ Warn loop. It now routes through theRetrierasDrop, matching watch-tower parity. The unknown-enum-marker case keeps its existing skip-and-keep behaviour, since an SDK upgrade could still pick that watch up. New testready_beyond_the_valid_to_horizon_drops_the_watchasserts zero submits, the watch dropped, and a warn logged.modules/examples/stop-loss/src/strategy.rs): aDuplicatedOrderrejection is now special-cased ahead ofclassify_api_errorviais_already_submitted. It writes thesubmitted:{uid}marker the dedup guard reads and idles, instead of re-posting the same order every block untilvalidTo. New regression testduplicate_rejection_records_receipt_and_idles.poll_oneand the keeper run loop'sDontTryAgainarm now log the four-byte revert selector and the node's message before a watch is permanently dropped, so a wrongly-dropped watch is triageable from the log alone.nexum_sdk::keeper::watch_keyhelper (no host turbofish needed) and switches call sites (including twap-monitor's test-onlywatch_key) over to it; drops the now-unneededshepherd-sdkdev-dependency fromshepherd-backtestandnexum-sdk-test.Why
Two skip-and-keep paths left a watch alive indefinitely once it reached a state that could never resolve: an
OrderCreationconstructor rejection is deterministic for the polled payload, so keeping the watch just re-polled and re-warned every block forever, and a duplicate-order rejection meant the orderbook already had the order, so re-posting on every subsequent block was pure waste. Both now converge on the watch-tower's actual behaviour (drop on an unrecoverable rejection, idle once the receipt is recorded), and the added selector/message logging means a dropped watch does not need to be reproduced to work out why it was dropped.Part of the M1 keeper sweep (#135).
Testing
Full gate battery run inside
nix develop, mirroringjust ci:cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features --locked -- -D warningsRUSTDOCFLAGS=-D warnings cargo doc --workspace --no-deps --lockedcargo build --release --target wasm32-wasip2 --lockedacross all example/module targetscargo test --workspace --all-features --no-fail-fast --locked, including the newready_beyond_the_valid_to_horizon_drops_the_watch,duplicate_rejection_records_receipt_and_idles, and the twap-monitor revert-selector log assertionsAI Assistance
Claude (Opus) used for the implementation.