Priority queue implementation

[helq]

On top of #414

This basic priority queue (commits 1ecf827 to b48262a) allows us to set a priority to patchsets between 000 to 999. By default, a patchset is given the priority 500.

These changes only introduce a simple priority queue but not a mechanism to change priorities yet. That is reserved for future PRs (coming today too).

The queue has been written so that it is efficient (we have an index in the db for fast queries) and naive. It is naive in which we assume the review queue will be emptied eventually and all patches will be reviewed eventually. So we are assuming no patchset will every be locked forever. This matches what we see in our CI/CD environment, where sashiko has highs (chugging thousands of reviews at the time) and lows (when only one or two patches are being reviewed).

[rgushchin]

🤖 AI Code Review

Here is an automated review of PR #415 ("Priority queue implementation"):

---

1. Regression Risks & Functional Bugs

🚨 1. Missed Wiring: recalculate_patchsets_priority is never called at startup

• Location: src/main.rs (lines 372–388) vs commit b48262a5ca0c.

• Issue: Commit b48262a5ca0c states:

  "Recalculate priorities for pending patchsets at startup via recalculate_patchsets_priority()."

  However, in src/main.rs, compiled_rules is compiled, but db.recalculate_patchsets_priority(&compiled_rules).await? is never called.

• Impact: On startup or restart (e.g. after operators update priority_rules in config, or upon migrating an existing DB where existing patchsets defaulted to 500), existing Pending, Incomplete, or Fetching patchsets are never evaluated against the priority rules and remain at default/old priority.

• Suggested Fix: Call db.recalculate_patchsets_priority(&compiled_rules).await?; in main() right after db.migrate().await?;.

---

🚨 2. Priority Inversion on Out-of-Order Multi-Part Ingestion via MIN(priority, ?)

• Location: src/db.rs (lines 1885 & 2168).

• Issue: When updating a patchset because an earlier part index arrived (e.g. [PATCH 0/N] cover letter arriving after [PATCH 1/N]), the SQL query executes:

  UPDATE patchsets SET subject = ?, subject_index = ?, priority = MIN(priority, ?) WHERE id = ?

  The reviewer queue orders by ORDER BY priority DESC, date ASC where higher number = higher priority (e.g. 1000 is high priority, 500 is default).

  MIN(priority, ?) picks the lower number (i.e. lower priority):

  • Suppose Patch 1/3 arrives first with subject "net/foo: driver fix" (matches no rules → priority = 500).
  • Later, Patch 0/3 (cover letter) arrives with subject "PRODKERNEL: critical security fixes" (matches rule → priority = 1000).
  • part_index < subject_index (0 < 1) triggers, updating the subject to the cover letter.
  • But MIN(500, 1000) evaluates to 500! The higher priority is discarded, and the patchset remains at 500 despite now having the high-priority subject.

• Impact: Any multi-part series where parts arrive out of order (common over NNTP / email delivery) will fail to acquire the higher priority of the cover letter. In addition, if recalculate_patchsets_priority is ever run, it evaluates calculate_priority(&subject, rules) and sets priority = 1000, causing inconsistent priority behavior between runtime ingestion and startup recalculation.

• Suggested Fix: When updating the subject because a lower part_index arrived, set priority = ? to match the newly authoritative subject rather than capping via MIN.

---

⚠️ 3. Silent Loop Abort on Error in recalculate_patchsets_priority

• Location: src/db.rs (lines 3887–3893).
• Issue:

  while let Ok(Some(row)) = rows.next().await {
      let id: i64 = row.get(0)?;
      ...
  }

  If rows.next().await returns Err(e), the while let Ok(...) condition fails silently, aborting the iteration early without propagating or logging the error.
• Suggested Fix: Use while let Some(row) = rows.next().await? so errors propagate.

---

2. Security & Safety

• ReDoS Protection: Safe. The implementation uses Rust's regex crate which is based on finite automata and guarantees $O(m \times n)$ worst-case time without backtracking.
• SQL Injection: Safe. Parameterized queries (libsql::params!) are used everywhere.

---

3. Architectural & Operational Notes

1. Starvation Risk: ORDER BY priority DESC, date ASC means a steady stream of high-priority patchsets can indefinitely starve default/low-priority patchsets. As noted in the PR description this is an intentional simple model for now, but an aging/anti-starvation mechanism will be helpful under sustained load.
2. Rule Order Precedence ("Last Match Wins"): calculate_priority loops through priority_rules where later matching rules overwrite earlier ones. This should be explicitly documented in docs/configuration.md.
3. Documentation: docs/configuration.md and Settings.toml should be updated to include examples for [[review.priority_rules]].

[helq]

Thank you for your comments I have revised the issues and cleaned up the commits. I have also sorted out all issues with the new db versioning :)

[rgushchin]

🤖 AI Code Review

Here is a summary of the issues to address before merging PR #415:

🚨 Blocking Issues

1. Submodule pointer error in third_party/linux
   • Commit 78fec789e29a accidentally updated third_party/linux to 6bea6b1f81acb0ffbd1da38c8c795c59050dc304.
   • This commit SHA does not exist on upstream torvalds/linux, causing git submodule update / recursive clone failures (upload-pack: not our ref).
   • Action: Revert third_party/linux back to 3609fa95fb0f2c1b099e69e56634edb8fc03f87c.

---

⚠️ Non-Blocking Cleanups & Notes

2. Outdated commit message in 5e6dc6da49d5

   • The commit message still mentions MIN(priority, ?) semantics from the earlier revision. Please update it to reflect the current MAX(base_priority, ?) and priority_cap design.

3. Asymmetric deprioritization for rules with priority < 500 (FYI)

   • Priority elevation (> 500) works across all part arrival orders. However, if a rule assigns a priority < 500 to a cover letter that arrives after an unclassified patch part, MAX(500, low_priority) will evaluate to 500. Not a concern if rules are solely used for elevating priority, but worth noting for future iterations.

[helq]

I've cleaned up the issues mentioned with third/party, including the asymmetric deprioritization note which was an excellent point for the agent to notice