fix(spider-storage::db): Avoid locking secondary indexes when scanning rows for deletion (fixes #371).

[sitaowang1998]

Description

This PR fixes #371.

This PR avoid deadlock in delete__expired_terminated_jobs and get_dead_execution_managers by:

1. Scan the table using the secondary index without holding exclusive locks on the index.
2. Lock the primary index and recheck the states.
   delete_expired_terminated_jobs doesn't need to recheck the state according to the program invariants (job in terminated state remains in the state & time only moves forward in db), but we still recheck the state just in case.
3. Update state/delete row.

Checklist

• The PR satisfies the contribution guidelines.
• This is a breaking change and that has been indicated in the PR title, OR this isn't a
  breaking change.
• Necessary docs have been updated, OR no docs need to be updated.

Validation performed

• GitHub workflows pass.

Summary by CodeRabbit

Summary by CodeRabbit

• Bug Fixes
  • Improved cleanup for expired terminated jobs by identifying eligible job IDs first, then deleting only matching rows with an explicit terminal-state check and consistency validation.
  • Increased reliability of dead execution-manager detection by confirming candidates before marking them as dead, reducing race-condition impact.
• Tests
  • Added a new (ignored) concurrency stress test to verify liveness operations complete correctly under heavy parallel updates.

[coderabbitai]

Walkthrough

This PR changes MariaDB job-expiry cleanup and dead execution-manager detection to use unlocked candidate selection followed by later confirmation or deletion, and adds concurrency test coverage for the liveness path.

Changes

Two-phase select-confirm-act query rework

Layer / File(s)	Summary
Expired job deletion confirmation components/spider-storage/src/db/mariadb.rs	The expired-job candidate query drops FOR UPDATE, and the deletion loop deletes only the candidate IDs with a terminal-state predicate and row-count validation.
Dead execution manager confirmation components/spider-storage/src/db/mariadb.rs	Dead-manager handling now runs in a READ COMMITTED transaction helper, selects stale candidates without locking, re-checks them in locked primary-key chunks, updates only confirmed rows to Dead, and returns the confirmed IDs.
Concurrency test coverage components/spider-storage/tests/mariadb_test.rs	An ignored multi-thread test adds concurrent heartbeat and dead-manager calls, with MariaDB connection limits adjusted for the spawned tasks and joined results checked for expected outcomes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant delete_expired_terminated_jobs
  participant run_read_committed_tx
  participant get_dead_execution_managers
  participant Database

  delete_expired_terminated_jobs->>Database: SELECT expired terminated job IDs (no lock)
  Database-->>delete_expired_terminated_jobs: candidate IDs
  delete_expired_terminated_jobs->>Database: DELETE ... WHERE id IN (...) AND state IN (...)
  Database-->>delete_expired_terminated_jobs: rows_affected

  get_dead_execution_managers->>run_read_committed_tx: run helper in READ COMMITTED tx
  run_read_committed_tx->>Database: set transaction isolation
  get_dead_execution_managers->>Database: SELECT stale Alive manager IDs (no lock)
  Database-->>get_dead_execution_managers: candidate IDs
  get_dead_execution_managers->>Database: SELECT ... FOR UPDATE on chunks
  Database-->>get_dead_execution_managers: confirmed IDs
  get_dead_execution_managers->>Database: UPDATE confirmed rows to Dead

Loading

Possibly related PRs

• y-scope/spider#337: Also changes MariaDB query and schema behaviour in components/spider-storage/src/db/mariadb.rs, affecting the same liveness and cleanup paths.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The delete_expired_terminated_jobs refactor is not covered by #371's EM liveness deadlock and appears to be extra scope.	Split the job-deletion deadlock fix into a separate PR or link the relevant issue so the scope matches the stated objective.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The change removes the lock-order inversion in get_dead_execution_managers and rechecks rows under PRIMARY, matching #371.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main fix: avoiding secondary-index locking during row scans to prevent MariaDB deadlocks.

✨ 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.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@components/spider-storage/src/db/mariadb.rs`:
- Around line 359-407: The batch deletion loop can livelock in the MariaDB
cleanup path because the non-locking candidate scan can keep returning the same
full batch while `confirmed_ids` stays empty under the default transaction
isolation. Fix the logic in this deletion routine by breaking the loop when
`confirmed_ids` is empty, or by restructuring the `SELECT_CANDIDATES_QUERY` scan
so it runs outside the long-lived transaction and only the confirm/delete work
stays in a short transaction. Keep the existing `select_for_update_query`,
`confirmed_ids`, and `DELETE_BATCH_SIZE` flow, but ensure a full batch with no
confirmations cannot spin forever.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: dcad76fc-e8eb-47a6-a976-0517355b4dcd

📥 Commits

Reviewing files that changed from the base of the PR and between 4ace626 and 6198ab5.

📒 Files selected for processing (1)

• components/spider-storage/src/db/mariadb.rs

[LinZhihao-723]

Committed small variable renaming changes.

[coderabbitai]

🧹 Nitpick comments (1)

components/spider-storage/src/db/mariadb.rs (1)

357-399: 🚀 Performance & Scalability | 🔵 Trivial

Consider committing per batch instead of one transaction spanning all batches.

The whole batched loop runs inside a single transaction (begin at Line 357, commit at Line 401). Every DELETE takes row locks that are held until the final commit, so a large cleanup accumulates locks on all deleted rows across all batches — which works against this PR's goal of reducing lock contention. Committing (and re-beginning) per batch would bound lock duration and also refresh the read snapshot each iteration. This is optional and depends on your durability/atomicity expectations for the cleanup.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@components/spider-storage/src/db/mariadb.rs` around lines 357 - 399, The
batched cleanup in the MariaDB delete flow currently holds a single transaction
open across all batches, so row locks from each DELETE remain until the final
commit. Update the transaction handling in the job-deletion logic around the
loop in the MariaDB storage method to commit each batch separately and start a
new transaction for the next batch, while preserving the existing candidate
selection, delete verification, and deleted_job_ids accumulation behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@components/spider-storage/src/db/mariadb.rs`:
- Around line 357-399: The batched cleanup in the MariaDB delete flow currently
holds a single transaction open across all batches, so row locks from each
DELETE remain until the final commit. Update the transaction handling in the
job-deletion logic around the loop in the MariaDB storage method to commit each
batch separately and start a new transaction for the next batch, while
preserving the existing candidate selection, delete verification, and
deleted_job_ids accumulation behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c7a7e5a1-3134-45ed-bc93-0a5610639051

📥 Commits

Reviewing files that changed from the base of the PR and between 4e0d7aa and e3ead46.

📒 Files selected for processing (1)

• components/spider-storage/src/db/mariadb.rs

[LinZhihao-723]

Directly modified the PR title.

• Since the scope of spider-storage is getting too large now, let's specify which component we touch using the namespace convention (in this case, spider-storage::db).