brooktrails/pr-reviewer
2
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions

fix: update DB when ignored events were previously persisted as queued #4

Merged
rcsheets merged 2 commits from fix/stuck-queued-events into main 2026-03-20 17:11:56 +00:00
Conversation 5 Commits 2 Files changed 1 +23 -1
rcsheets commented 2026-03-20 16:39:44 +00:00
Owner
Copy link

When _maybe_re_review decides to skip, it sets status to "ignored" and
calls record(). But record() routed ignored events to in-memory only,
leaving the DB row stuck as "queued" forever.

Now record() checks if an ignored event already exists in the DB and
updates it there instead. Also cleans up existing stuck rows on startup.

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

When _maybe_re_review decides to skip, it sets status to "ignored" and calls record(). But record() routed ignored events to in-memory only, leaving the DB row stuck as "queued" forever. Now record() checks if an ignored event already exists in the DB and updates it there instead. Also cleans up existing stuck rows on startup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When _maybe_re_review decides to skip, it sets status to "ignored" and
calls record(). But record() routed ignored events to in-memory only,
leaving the DB row stuck as "queued" forever.

Now record() checks if an ignored event already exists in the DB and
updates it there instead. Also cleans up existing stuck rows on startup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pr-reviewer-bot commented 2026-03-20 16:39:51 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.8.0 | Full Review | anthropic | tracking id r-bd7856-8469d6

Status: ✅ Completed

Looking at this pull request, I can see it addresses a specific bug where events that were previously persisted to the database as "queued" would get stuck in that state when later marked as "ignored".

Code Review

Positive Aspects

  1. Clear Problem Definition: The PR description clearly explains the issue - when _maybe_re_review decides to skip an event and sets status to "ignored", the database row remained stuck as "queued" while only the in-memory store was updated.

  2. Two-Part Solution: The fix appropriately handles both existing stuck events (via migration) and prevents future occurrences (via the logic change).

  3. Migration Strategy: The migration in MIGRATE_COLUMNS provides a clean way to fix existing stuck rows with a descriptive error message.

Issues and Concerns

Database Connection Handling (tracker.py:98-119)

There's a problematic pattern in the connection handling:

try:
    with conn:
        row = conn.execute(...)
    if row:
        with conn:  # Using connection again after first context manager
            conn.execute(...)
        conn.close()  # Manual close inside try block
        return
except Exception:
    # ...
finally:
    if not conn.closed:
        conn.close()  # Another manual close

Problems:

  1. The connection is used in multiple with blocks, which is confusing and potentially error-prone
  2. Manual conn.close() calls within try blocks bypass the finally block's cleanup
  3. The conn.closed property might not be reliable across all database drivers

Suggested fix:

conn = _get_conn()
if conn:
    try:
        with conn:
            row = conn.execute(
                "SELECT 1 FROM review_events WHERE id = %s", (event.id,)
            ).fetchone()
            if row:
                # Already in DB — update it there
                conn.execute(
                    "UPDATE review_events SET status = %s, error = %s, duration_ms = %s WHERE id = %s",
                    (event.status, event.ignore_reason or event.error, event.duration_ms, event.id),
                )
                return
    except Exception:
        logger.exception("Failed to update ignored event %s in database", event.id)
    finally:
        conn.close()

Missing Field in get_events() (tracker.py:381)

The get_events() function doesn't include the request_id field when constructing ReviewEvent objects from database rows, even though it selects it. This could cause issues if the field is accessed later.

# Current - missing request_id
db_events.append(ReviewEvent(
    id=row[0], timestamp=row[1], repo=row[2], pr_number=row[3],
    action=row[4], tier=row[5], status=row[6], backend=row[7],
    input_tokens=row[8], output_tokens=row[9], diff_chars=row[10],
    error=row[11], duration_ms=row[12],
))

# Should be:
db_events.append(ReviewEvent(
    id=row[0], timestamp=row[1], repo=row[2], pr_number=row[3],
    action=row[4], tier=row[5], status=row[6], backend=row[7],
    input_tokens=row[8], output_tokens=row[9], diff_chars=row[10],
    error=row[11], duration_ms=row[12], request_id=row[13],
))

Logic Consideration

The current logic adds ignored events to _ignored_events even when they're successfully updated in the database. This could lead to duplicate tracking. Consider whether ignored events that are successfully persisted to the database should still be added to the in-memory list.

Minor Issues

  1. Error Field Usage: Using event.ignore_reason or event.error is clever but could be confusing. Consider being more explicit about which field should be used when.

  2. Migration Message: The migration message "stuck queued event (fixed in 0.8.2)" hardcodes a version number, which might become outdated.

Security and Performance

  • No security vulnerabilities identified
  • Database queries are properly parameterized
  • Performance impact is minimal (one additional SELECT query for ignored events that were previously persisted)

Test Coverage

The changes would benefit from tests covering:

  • Ignored events that were previously queued get updated in the database
  • The migration correctly fixes existing stuck events
  • Connection handling edge cases

Summary

The core logic of the fix is sound and addresses the described problem effectively. However, there are implementation issues with database connection handling and a missing field in the get_events() function that should be addressed before merging.

Verdict: Request Changes

The connection handling pattern needs to be fixed to avoid potential resource leaks and the missing request_id field in get_events() should be added. These are straightforward fixes that don't affect the core solution.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.8.0 | Full Review | anthropic | tracking id `r-bd7856-8469d6`* **Status:** ✅ Completed --- Looking at this pull request, I can see it addresses a specific bug where events that were previously persisted to the database as "queued" would get stuck in that state when later marked as "ignored". ## Code Review ### **Positive Aspects** 1. **Clear Problem Definition**: The PR description clearly explains the issue - when `_maybe_re_review` decides to skip an event and sets status to "ignored", the database row remained stuck as "queued" while only the in-memory store was updated. 2. **Two-Part Solution**: The fix appropriately handles both existing stuck events (via migration) and prevents future occurrences (via the logic change). 3. **Migration Strategy**: The migration in `MIGRATE_COLUMNS` provides a clean way to fix existing stuck rows with a descriptive error message. ### **Issues and Concerns** #### **Database Connection Handling (tracker.py:98-119)** There's a problematic pattern in the connection handling: ```python try: with conn: row = conn.execute(...) if row: with conn: # Using connection again after first context manager conn.execute(...) conn.close() # Manual close inside try block return except Exception: # ... finally: if not conn.closed: conn.close() # Another manual close ``` **Problems:** 1. The connection is used in multiple `with` blocks, which is confusing and potentially error-prone 2. Manual `conn.close()` calls within try blocks bypass the finally block's cleanup 3. The `conn.closed` property might not be reliable across all database drivers **Suggested fix:** ```python conn = _get_conn() if conn: try: with conn: row = conn.execute( "SELECT 1 FROM review_events WHERE id = %s", (event.id,) ).fetchone() if row: # Already in DB — update it there conn.execute( "UPDATE review_events SET status = %s, error = %s, duration_ms = %s WHERE id = %s", (event.status, event.ignore_reason or event.error, event.duration_ms, event.id), ) return except Exception: logger.exception("Failed to update ignored event %s in database", event.id) finally: conn.close() ``` #### **Missing Field in get_events() (tracker.py:381)** The `get_events()` function doesn't include the `request_id` field when constructing `ReviewEvent` objects from database rows, even though it selects it. This could cause issues if the field is accessed later. ```python # Current - missing request_id db_events.append(ReviewEvent( id=row[0], timestamp=row[1], repo=row[2], pr_number=row[3], action=row[4], tier=row[5], status=row[6], backend=row[7], input_tokens=row[8], output_tokens=row[9], diff_chars=row[10], error=row[11], duration_ms=row[12], )) # Should be: db_events.append(ReviewEvent( id=row[0], timestamp=row[1], repo=row[2], pr_number=row[3], action=row[4], tier=row[5], status=row[6], backend=row[7], input_tokens=row[8], output_tokens=row[9], diff_chars=row[10], error=row[11], duration_ms=row[12], request_id=row[13], )) ``` #### **Logic Consideration** The current logic adds ignored events to `_ignored_events` even when they're successfully updated in the database. This could lead to duplicate tracking. Consider whether ignored events that are successfully persisted to the database should still be added to the in-memory list. ### **Minor Issues** 1. **Error Field Usage**: Using `event.ignore_reason or event.error` is clever but could be confusing. Consider being more explicit about which field should be used when. 2. **Migration Message**: The migration message "stuck queued event (fixed in 0.8.2)" hardcodes a version number, which might become outdated. ### **Security and Performance** - No security vulnerabilities identified - Database queries are properly parameterized - Performance impact is minimal (one additional SELECT query for ignored events that were previously persisted) ### **Test Coverage** The changes would benefit from tests covering: - Ignored events that were previously queued get updated in the database - The migration correctly fixes existing stuck events - Connection handling edge cases ## Summary The core logic of the fix is sound and addresses the described problem effectively. However, there are implementation issues with database connection handling and a missing field in the `get_events()` function that should be addressed before merging. **Verdict: Request Changes** The connection handling pattern needs to be fixed to avoid potential resource leaks and the missing `request_id` field in `get_events()` should be added. These are straightforward fixes that don't affect the core solution.
pr-reviewer-bot commented 2026-03-20 16:39:51 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.8.0 | Full Review | anthropic | tracking id r-bd7851-446375

Status: ✅ Completed

I'll provide a detailed review of this pull request that fixes stuck "queued" events in the database.

Overall Assessment

This is a well-targeted bug fix that addresses a legitimate issue where events marked as "ignored" during re-review decisions weren't being properly updated in the database, leaving them stuck with "queued" status. The solution is conceptually sound and includes both a fix for the ongoing issue and cleanup of existing problematic data.

Issues to Address

1. Database Connection Resource Management (tracker.py:97-119)

The connection handling in the new ignored event logic has several issues:

  • Double close potential: Line 113 calls conn.close() but then the finally block on line 118 checks if not conn.closed: and closes again. This creates redundant calls.
  • Inconsistent pattern: The rest of the codebase uses try/finally with a single close in the finally block. This new code deviates from that pattern.
  • Transaction scope confusion: There are nested with conn: blocks (lines 101 and 107) which is unnecessary and potentially confusing.

Recommendation: Refactor to match the existing pattern:

conn = _get_conn()
if conn:
    try:
        with conn:
            row = conn.execute(
                "SELECT 1 FROM review_events WHERE id = %s", (event.id,)
            ).fetchone()
            if row:
                conn.execute(
                    "UPDATE review_events SET status = %s, error = %s, duration_ms = %s WHERE id = %s",
                    (event.status, event.ignore_reason or event.error, event.duration_ms, event.id),
                )
                return
    except Exception:
        logger.exception("Failed to update ignored event %s in database", event.id)
    finally:
        conn.close()

2. Logic Flow Issue (tracker.py:113-119)

The early return on line 113 bypasses the finally block, which means the connection gets closed there, but then execution continues to the in-memory storage logic (lines 121-124). This creates a subtle bug where ignored events that were updated in the database also get added to the in-memory store.

Expected behavior: If an ignored event exists in DB and gets updated there, it should NOT also be added to _ignored_events.
Current behavior: Due to the early return, the in-memory storage code is unreachable for DB-updated events, which is actually correct but makes the code structure confusing.

3. Missing Field in get_events() (tracker.py:346)

The get_events() function doesn't populate the request_id field when creating ReviewEvent objects from database rows, even though it selects it. Line 346 shows only 13 fields being passed to the constructor, but request_id is the 14th field (row[13]).

Bug: This will cause request_id to always be empty string for events loaded from the database.

4. Data Migration Concerns (tracker.py:66)

The migration SQL UPDATE review_events SET status = 'ignored', error = 'stuck queued event (fixed in 0.8.2)' WHERE status = 'queued'; is aggressive - it updates ALL queued events to ignored status. This could potentially affect legitimately queued events that happen to be in the database when the migration runs.

Risk: If there are any genuinely queued events during deployment, they'll be incorrectly marked as ignored.

Positive Aspects

  1. Clear problem identification: The PR correctly identifies that record() was routing ignored events to in-memory only storage even when they already existed in the database.

  2. Comprehensive solution: Addresses both the ongoing issue and cleans up existing problematic data.

  3. Good error handling: Includes proper exception logging for database operations.

  4. Maintains backward compatibility: The fix doesn't break existing functionality.

  5. Appropriate use of database features: Uses a simple SELECT to check existence before UPDATE, which is a reasonable approach for this use case.

Minor Issues

  • Code readability: The nested database operations make the record() function more complex. Consider extracting the ignored event update logic into a separate helper function.

  • Performance consideration: The solution adds an extra database query for every ignored event. Given that ignored events are likely common, this could have a minor performance impact. However, this is probably acceptable given the correctness benefit.

Recommendations

  1. Fix the connection management pattern to be consistent with the rest of the codebase
  2. Fix the missing request_id field in get_events()
  3. Consider making the migration more conservative (e.g., only update events older than a certain threshold)
  4. Consider extracting the ignored event update logic into a helper function for better readability

Verdict: Request Changes

While this addresses a real bug and the approach is sound, the connection management issues and missing field bug should be addressed before merging.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.8.0 | Full Review | anthropic | tracking id `r-bd7851-446375`* **Status:** ✅ Completed --- I'll provide a detailed review of this pull request that fixes stuck "queued" events in the database. ## Overall Assessment This is a well-targeted bug fix that addresses a legitimate issue where events marked as "ignored" during re-review decisions weren't being properly updated in the database, leaving them stuck with "queued" status. The solution is conceptually sound and includes both a fix for the ongoing issue and cleanup of existing problematic data. ## Issues to Address ### 1. Database Connection Resource Management (tracker.py:97-119) The connection handling in the new ignored event logic has several issues: - **Double close potential**: Line 113 calls `conn.close()` but then the `finally` block on line 118 checks `if not conn.closed:` and closes again. This creates redundant calls. - **Inconsistent pattern**: The rest of the codebase uses `try/finally` with a single close in the finally block. This new code deviates from that pattern. - **Transaction scope confusion**: There are nested `with conn:` blocks (lines 101 and 107) which is unnecessary and potentially confusing. **Recommendation**: Refactor to match the existing pattern: ```python conn = _get_conn() if conn: try: with conn: row = conn.execute( "SELECT 1 FROM review_events WHERE id = %s", (event.id,) ).fetchone() if row: conn.execute( "UPDATE review_events SET status = %s, error = %s, duration_ms = %s WHERE id = %s", (event.status, event.ignore_reason or event.error, event.duration_ms, event.id), ) return except Exception: logger.exception("Failed to update ignored event %s in database", event.id) finally: conn.close() ``` ### 2. Logic Flow Issue (tracker.py:113-119) The early `return` on line 113 bypasses the `finally` block, which means the connection gets closed there, but then execution continues to the in-memory storage logic (lines 121-124). This creates a subtle bug where ignored events that were updated in the database also get added to the in-memory store. **Expected behavior**: If an ignored event exists in DB and gets updated there, it should NOT also be added to `_ignored_events`. **Current behavior**: Due to the early return, the in-memory storage code is unreachable for DB-updated events, which is actually correct but makes the code structure confusing. ### 3. Missing Field in get_events() (tracker.py:346) The `get_events()` function doesn't populate the `request_id` field when creating `ReviewEvent` objects from database rows, even though it selects it. Line 346 shows only 13 fields being passed to the constructor, but `request_id` is the 14th field (row[13]). **Bug**: This will cause `request_id` to always be empty string for events loaded from the database. ### 4. Data Migration Concerns (tracker.py:66) The migration SQL `UPDATE review_events SET status = 'ignored', error = 'stuck queued event (fixed in 0.8.2)' WHERE status = 'queued';` is aggressive - it updates ALL queued events to ignored status. This could potentially affect legitimately queued events that happen to be in the database when the migration runs. **Risk**: If there are any genuinely queued events during deployment, they'll be incorrectly marked as ignored. ## Positive Aspects 1. **Clear problem identification**: The PR correctly identifies that `record()` was routing ignored events to in-memory only storage even when they already existed in the database. 2. **Comprehensive solution**: Addresses both the ongoing issue and cleans up existing problematic data. 3. **Good error handling**: Includes proper exception logging for database operations. 4. **Maintains backward compatibility**: The fix doesn't break existing functionality. 5. **Appropriate use of database features**: Uses a simple SELECT to check existence before UPDATE, which is a reasonable approach for this use case. ## Minor Issues - **Code readability**: The nested database operations make the `record()` function more complex. Consider extracting the ignored event update logic into a separate helper function. - **Performance consideration**: The solution adds an extra database query for every ignored event. Given that ignored events are likely common, this could have a minor performance impact. However, this is probably acceptable given the correctness benefit. ## Recommendations 1. Fix the connection management pattern to be consistent with the rest of the codebase 2. Fix the missing `request_id` field in `get_events()` 3. Consider making the migration more conservative (e.g., only update events older than a certain threshold) 4. Consider extracting the ignored event update logic into a helper function for better readability ## Verdict: Request Changes While this addresses a real bug and the approach is sound, the connection management issues and missing field bug should be addressed before merging.
- Simplified DB connection handling to single `with conn:` block
- Don't add to in-memory list when successfully updated in DB
- Added missing request_id field in get_events()
- Made error detail field selection more explicit
- Removed hardcoded version from migration message

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rcsheets commented 2026-03-20 16:43:35 +00:00
Author
Owner
Copy link

@pr-reviewer-bot help

@pr-reviewer-bot help
pr-reviewer-bot commented 2026-03-20 16:43:36 +00:00
Collaborator
Copy link

pr-reviewer v0.8.0 | tracking id r-bd7938-f5ecb4

I'm pr-reviewer, an automated code review bot.

Commands (use in a PR comment):

Command What it does
@pr-reviewer-bot review Request a code review
@pr-reviewer-bot carefully review Request a thorough review with full file context
@pr-reviewer-bot help Show this message

Modifiers like careful, carefully, full, fully, thorough, or thoroughly can appear before or after review to request a more thorough review.

What happens automatically:

  • Safety check on every PR open/sync/reopen — a quick sanity check to catch obvious problems like exposed secrets, broken logic, or accidentally committed files. This is not a code review.
  • Full review when I'm assigned as a reviewer on the PR.
*[pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.8.0 | tracking id `r-bd7938-f5ecb4`* I'm **pr-reviewer**, an automated code review bot. **Commands** (use in a PR comment): | Command | What it does | |---|---| | `@pr-reviewer-bot review` | Request a code review | | `@pr-reviewer-bot carefully review` | Request a thorough review with full file context | | `@pr-reviewer-bot help` | Show this message | Modifiers like `careful`, `carefully`, `full`, `fully`, `thorough`, or `thoroughly` can appear before or after `review` to request a more thorough review. **What happens automatically:** - **Safety check** on every PR open/sync/reopen — a quick sanity check to catch obvious problems like exposed secrets, broken logic, or accidentally committed files. This is not a code review. - **Full review** when I'm assigned as a reviewer on the PR.
rcsheets commented 2026-03-20 16:44:19 +00:00
Author
Owner
Copy link

@pr-reviewer-bot full review

@pr-reviewer-bot full review
rcsheets deleted branch fix/stuck-queued-events 2026-03-20 17:12:01 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
pr-reviewer-bot
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
brooktrails/pr-reviewer!4
No description provided.