Skip to content

Add MRT recovery state migration#810

Open
Perkeloni wants to merge 1 commit into
roostorg:mainfrom
Perkeloni:maco/ISSUE-146/mrt-recovery-migration
Open

Add MRT recovery state migration#810
Perkeloni wants to merge 1 commit into
roostorg:mainfrom
Perkeloni:maco/ISSUE-146/mrt-recovery-migration

Conversation

@Perkeloni

@Perkeloni Perkeloni commented Jun 19, 2026

Copy link
Copy Markdown

Context & Requests for Reviewers

Adds the mrt_queue_recovery_state migration only. This should land before the recovery code PRs.

Tests

Not run; migration only.

Rollout Plan

Merge this first, then land the recovery code stack on top of it.

Summary by CodeRabbit

  • Chores
    • Added durable tracking for manual review job queue recovery and retry progress, including retry counts and last error details.
    • Introduced automatic update timestamps and an indexed status field to support efficient recovery state management.
    • Ensured related recovery records are removed automatically when their parent job is deleted.

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

A single SQL migration adds the manual_review_tool.mrt_queue_recovery_state table to track retry and recovery state for MRT Redis backfill jobs. The table includes status (PENDING/FAILED), a non-negative retry_count, last_error, timestamps, a cascading foreign key to job_creations, and an index on status. A PL/pgSQL trigger automatically maintains the updated_at timestamp on each row modification.

Changes

MRT Queue Recovery State Migration

Layer / File(s) Summary
Table schema and recovery state tracking
db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql
Creates the manual_review_tool.mrt_queue_recovery_state table with job_id as PK, status constrained to PENDING/FAILED, a non-negative retry_count defaulting to 0, last_error, created_at/updated_at timestamps, a cascading FK to job_creations(id), an index on status, and descriptive table/column comments.
Timestamp lifecycle trigger
db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql
Adds a PL/pgSQL trigger function that sets updated_at to now() before row updates, and creates a corresponding BEFORE UPDATE trigger on mrt_queue_recovery_state to execute it.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Add MRT recovery state migration' clearly and concisely describes the primary change: adding a migration for MRT recovery state functionality.
Description check ✅ Passed The description includes all required sections (Context & Requests for Reviewers, Tests, and Rollout Plan) with appropriate details for a migration-only PR.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ 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

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql (1)

27-28: ⚖️ Poor tradeoff

Consider indexes for common query patterns.

The status index is appropriate for finding PENDING jobs. If the recovery process will also query by org_id, queue_id, or combinations like (status, org_id), consider adding those indexes for performance. This can be deferred until actual query patterns are known.

🤖 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
`@db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql`
around lines 27 - 28, Review the recovery process code to identify all common
query patterns against the mrt_queue_recovery_state table, particularly checking
if queries filter by org_id, queue_id, or combinations of status with these
fields. Once the actual query patterns are confirmed, add additional indexes to
the migration script for frequently used fields or composite indexes like
(status, org_id) to optimize query performance, or document that this
optimization can be deferred until query patterns stabilize in production.
🤖 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
`@db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql`:
- Line 18: The `mrt_queue_recovery_state` table has an `updated_at` column that
defaults to `now()` but lacks a trigger to update it during row modifications.
Add a trigger function named `update_mrt_queue_recovery_state_updated_at()` in
the `manual_review_tool` schema that sets `NEW.updated_at` to the current
timestamp, then create a BEFORE UPDATE trigger named
`mrt_queue_recovery_state_updated_at_trigger` on the `mrt_queue_recovery_state`
table that executes this function for each row being updated. This should be
added after the table and index creation in the migration script.
- Around line 8-19: The migration file creates the table
mrt_queue_recovery_state but is missing the required GRANT statements to allow
application roles to access it. After the CREATE TABLE statement for
mrt_queue_recovery_state, add GRANT statements that grant appropriate
permissions (SELECT, INSERT, UPDATE, DELETE) to the application role, using
CURRENT_USER syntax instead of hardcoded role names, as per coding guidelines
for supporting any postgres user.

---

Nitpick comments:
In
`@db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql`:
- Around line 27-28: Review the recovery process code to identify all common
query patterns against the mrt_queue_recovery_state table, particularly checking
if queries filter by org_id, queue_id, or combinations of status with these
fields. Once the actual query patterns are confirmed, add additional indexes to
the migration script for frequently used fields or composite indexes like
(status, org_id) to optimize query performance, or document that this
optimization can be deferred until query patterns stabilize in production.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a1b8faf4-4c92-429e-aedb-cc0a87f1c5b2

📥 Commits

Reviewing files that changed from the base of the PR and between beaaaf7 and 2e303e3.

📒 Files selected for processing (1)
  • db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql

@Perkeloni Perkeloni force-pushed the maco/ISSUE-146/mrt-recovery-migration branch from 986dd48 to aca6460 Compare June 19, 2026 19:36
@Perkeloni Perkeloni force-pushed the maco/ISSUE-146/mrt-recovery-migration branch from aca6460 to 48c0bff Compare June 19, 2026 19:40

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql (2)

14-14: ⚡ Quick win

Consider adding DEFAULT 'PENDING' to the status column.

Since PENDING is the initial state for recovery entries (per the column comment on line 52-53), adding a default would simplify inserts and make the intent explicit in the schema.

💡 Proposed enhancement
-  status text NOT NULL CHECK (status IN ('PENDING', 'FAILED')),
+  status text NOT NULL DEFAULT 'PENDING' CHECK (status IN ('PENDING', 'FAILED')),
🤖 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
`@db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql`
at line 14, The status column in the migration file currently lacks a default
value despite PENDING being the documented initial state for recovery entries.
Add `DEFAULT 'PENDING'` to the status column definition to set the default value
explicitly and simplify future inserts by automatically applying the initial
state when no status is specified.

9-9: Consider using character varying(255) for job_id to match the FK target type.

The job_id column uses text, but it references job_creations.id which is character varying(255). While PostgreSQL allows this FK relationship, aligning the types improves schema consistency. Other ID columns in the table (org_id, queue_id, etc.) should also match the types used in their corresponding source tables.

🤖 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
`@db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql`
at line 9, The job_id column is defined as text but should match the type of its
foreign key target job_creations.id which is character varying(255). Change the
job_id column definition from text to character varying(255) PRIMARY KEY.
Additionally, review other ID columns in the table (org_id, queue_id) and update
them to match the types used in their corresponding source tables to ensure
schema consistency throughout.

Source: Coding guidelines

🤖 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
`@db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql`:
- Line 14: The status column in the migration file currently lacks a default
value despite PENDING being the documented initial state for recovery entries.
Add `DEFAULT 'PENDING'` to the status column definition to set the default value
explicitly and simplify future inserts by automatically applying the initial
state when no status is specified.
- Line 9: The job_id column is defined as text but should match the type of its
foreign key target job_creations.id which is character varying(255). Change the
job_id column definition from text to character varying(255) PRIMARY KEY.
Additionally, review other ID columns in the table (org_id, queue_id) and update
them to match the types used in their corresponding source tables to ensure
schema consistency throughout.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1268e2a0-ab4c-40bf-b740-acf6a684fbd8

📥 Commits

Reviewing files that changed from the base of the PR and between 2e303e3 and 48c0bff.

📒 Files selected for processing (1)
  • db/src/scripts/api-server-pg/2026.06.17T03.57.25.add_mrt_queue_recovery_state.sql

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant