fix: exclude hyphenated terminal order statuses

[21Mill]

EXCLUDED_ORDER_STATUSES and TERMINAL_ORDER_STATUSES in src/db.rs list the four multi-word order statuses without hyphens, but Status' Display impl serializes them as kebab-case, and that is the form actually stored in the orders table.

// src/db.rs — before
const EXCLUDED_ORDER_STATUSES: &str = "'expired','success','canceled','dispute',
    'canceledbyadmin','completedbyadmin','settledbyadmin','cooperativelycanceled'";

// mostro-core 0.14.5, order.rs
Status::CanceledByAdmin       => write!(f, "canceled-by-admin"),
Status::SettledByAdmin        => write!(f, "settled-by-admin"),
Status::CompletedByAdmin      => write!(f, "completed-by-admin"),
Status::CooperativelyCanceled => write!(f, "cooperatively-canceled"),

RestoredOrdersInfo::status even documents the contract the constants break — "Current status of the order, serialized as kebab-case" (message.rs:490).

The four single-word entries (expired, success, canceled, dispute) do match, so both NOT IN filters keep working for those and fail silently for the rest. Two other constants in the same file — ACTIVE_DISPUTE_STATUSES ('in-progress') and PRETRADE_STATUSES ('waiting-taker-bond') — are hyphenated correctly, which is what makes this look like a typo rather than an alternative encoding.

Impact

find_user_orders_by_master_key (restore session) returns orders that are already over. On a live node, 30 rows qualified — months-old cooperatively-canceled and canceled-by-admin orders — and they were handed back to their owners as restorable session state for trades their clients no longer track.

This also has a knock-on effect: find_user_orders_by_master_key has no LIMIT, while the follow-up orders action rejects the whole request when ids.len() > max_orders_per_response (src/app/orders.rs). Users whose accumulated dead orders push the restore list past the cap get a blanket too_many_requests and cannot refresh at all — the server offers a list its own limit will not accept back. That is how this bug was found.

find_active_trade_pubkeys (Phase 2 anti-spam gate) never drops the trade pubkeys of admin-closed or cooperatively-canceled orders, because those orders never count as terminal. Their keys stay in the fast-pathed "known keys" set indefinitely, which is precisely what TERMINAL_ORDER_STATUSES exists to prevent.

Verification against real data

Same query, same database, only the status list differs:

status	before	after
cooperatively-canceled	28	—
canceled-by-admin	2	—
pending	12	12
waiting-maker-bond	5	5
settled-hold-invoice	4	4
waiting-taker-bond	1	1

30 terminal orders stop being restored; every genuinely active order is untouched.

Why the existing tests missed it

find_active_trade_pubkeys_covers_active_and_disputed_excludes_terminal does test terminal exclusion, but only inserts 'success' and 'canceled' — the two spellings that happen to be correct. No test used a hyphenated status on the terminal side.

Both tests now cover every hyphenated status individually. Confirmed failing before the constant change and passing after:

---- find_user_orders_by_master_key_excludes_all_terminal_statuses stdout ----
assertion `left == right` failed: restore must return only the live order
  left: ["waiting-payment", "canceled-by-admin", "completed-by-admin",
         "settled-by-admin", "cooperatively-canceled"]
 right: ["waiting-payment"]

---- find_active_trade_pubkeys_covers_active_and_disputed_excludes_terminal stdout ----
creator_coop must NOT be known (terminal/resolved)

Full suite: 1187 passed, 0 failed. cargo fmt --check and cargo clippy --all-targets -- -D warnings are clean.

Interaction with #784

Worth flagging explicitly for reviewers. #784 ("Restore session loses pending bond payouts on terminal-status orders") describes the admin-cancel route on the assumption that 'canceledbyadmin' is already excluded. It is not — because of this bug — so that branch of #784 does not currently reproduce, while the admin-settle branch ('success', correctly spelled) does.

This change makes the two routes behave the same, which means the admin-cancel branch of #784 will start reproducing once this lands. It does not create that problem and does not make it worse in substance; it removes an accidental mask over half of it. Fixing #784 needs its own change to restore the bonds state machine, so it is deliberately out of scope here.

Summary by CodeRabbit

• Bug Fixes

  • Corrected handling of terminal order statuses with hyphenated values.
  • Terminal orders are now properly excluded from active trade-key and session-restore results.

• Tests

  • Added regression coverage for hyphenated terminal statuses.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ec23cdb0-1b69-41fd-9aed-4272e32975df

📥 Commits

Reviewing files that changed from the base of the PR and between 10fd6fd and 4d17c8d.

📒 Files selected for processing (1)

• src/db.rs

Included review availability: Your plan includes up to 2 reviews per rolling hour; 1 remains after this review.

---

Walkthrough

The change updates terminal order status constants to use hyphenated serialized values. Database tests verify active trade-key and restore-session queries exclude orders with these statuses.

Changes

Terminal status filtering

Layer / File(s)	Summary
Hyphenated terminal status constants src/db.rs	EXCLUDED_ORDER_STATUSES and TERMINAL_ORDER_STATUSES now contain hyphenated status values.
Filtering regression tests src/db.rs	Tests verify that hyphenated terminal orders are excluded from active trade keys and restore-session results.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 4d17c

This change corrects terminal-status filtering so resolved orders are excluded from restoration and active-key lookups; the reported tests and checks pass, and no actionable merge-blocking risk remains.

Possibly related PRs

• MostroP2P/mostro#869: Both changes update order-status filtering in src/db.rs, but target different queries and status-selection logic.

Suggested reviewers: grunch, arkanoider

Poem

I checked each status by moonlit light,
Hyphens placed the records right.
Terminal orders leave the key,
Restore finds the live one free.
— A database rabbit 🐇

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: excluding terminal orders that use hyphenated status values.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ToRyVand]

Reviewed and verified independently against main — everything in the description holds up. Details of what I checked rather than took on faith:

The premise. Confirmed Status' Display impl in mostro-core 0.14.5 (the version this repo pins in Cargo.toml) does emit the hyphenated forms — canceled-by-admin, settled-by-admin, completed-by-admin, cooperatively-canceled — while both constants on main carry the unhyphenated spellings. Real mismatch.

That it's a typo, not an alternative encoding. ACTIVE_DISPUTE_STATUSES ('in-progress') and PRETRADE_STATUSES ('waiting-taker-bond') in the same file are hyphenated correctly, which is the detail that settles it.

Reproduced the failure. Reverted TERMINAL_ORDER_STATUSES to the pre-fix spelling on your branch and ran find_active_trade_pubkeys_covers_active_and_disputed_excludes_terminal — fails, exactly as you documented. Restored the fix — passes. Same for the new restore-session test.

Completeness. Grepped all of src/ for any remaining unhyphenated occurrences of the four statuses: none. No other call site reintroduces the bug, and both constants feed the two functions you name (find_user_orders_by_master_key at the format! in db.rs, and find_active_trade_pubkeys).

Checks. Full suite 1187 passed / 0 failed, cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean.

One thing worth calling out as reviewer rather than as a request: flagging the #784 interaction up front — that this removes an accidental mask over half of it rather than creating anything — is the right call and saved me from having to work that out. Scoping the bonds state machine fix out of here is correct.

Note I'm a contributor, not a maintainer, so treat this as a technical second opinion for @grunch rather than a merge signal.

[AndreaDiazCorreia]

tACK

[AndreaDiazCorreia]

Reviewed and verified locally. The fix is correct and the tests are genuine regression tests — I reverted only the two constants while keeping the new tests, and both fail with exactly the output described; restoring the constants makes them pass.

Since the branch is based a few commits before 0.18.5, I re-ran everything against current main (a7249cc) with the patch applied rather than against the branch base:

cargo test --all-targets                   1223 passed; 0 failed; 2 ignored
cargo clippy --all-targets -- -D warnings  clean
cargo fmt --check                          clean

The patch applies to current main without conflict, and src/db.rs was untouched by the commits in between.

I also grepped src/, migrations/ and the docs for any remaining unhyphenated occurrence of the four statuses: the two constants in this diff were the only ones. Worth stating explicitly for whoever merges this — since nothing ever wrote those spellings, no rows carry them, so this needs no data migration. The bug was read-side only.

One nice side effect: TERMINAL_ORDER_STATUSES in src/db.rs now agrees with the typed [Status; 7] array in src/app/bond/db.rs:240. Those two had silently drifted apart for as long as this bug existed.

Follow-ups

Opened separately so they do not expand this PR's scope:

• refactor(db): derive SQL status lists from Status instead of hand-written literals #897 — the root cause is that these lists are hand-written SQL literals duplicating Status' Display, so a typo compiles, passes clippy, and silently stops filtering. src/app/bond/db.rs already does it right (typed array, bound parameters). Prevention, not live breakage.
• Restore session can return more orders than the orders action accepts #898 — find_user_orders_by_master_key has no LIMIT while src/app/orders.rs:26 rejects the whole batch above max_orders_per_response (default 10). This PR removes the dead-order inflation that made the cap easy to hit, but a user with more than 10 genuinely live orders still cannot refresh.

Before merging

Worth linking #784 from here. As the description says, this removes an accidental mask rather than creating a problem — but the practical effect is that the admin-cancel branch of #784 starts reproducing once this lands. Without the link that will read as a fresh regression to whoever hits it.

[Catrya]

ACK