Skip to content

fix(graph): roll back on COMMIT failure in batched link_edges#6

Merged
prom3theu5 merged 1 commit into
mainfrom
fix/commit-rollback
Jun 1, 2026
Merged

fix(graph): roll back on COMMIT failure in batched link_edges#6
prom3theu5 merged 1 commit into
mainfrom
fix/commit-rollback

Conversation

@prom3theu5

Copy link
Copy Markdown
Member

Addresses a Qodo review finding on the batched edge writer (#5).

Problem

LadybugGraphStore::link_edges rolls back on a mid-batch execution error, but the COMMIT path just returned the error without rolling back. A failed COMMIT leaves the transaction open, risking a dangling transaction/lock on the connection — asymmetric with the mid-batch error handling.

Fix

Make the COMMIT path symmetric: on commit failure, do a best-effort ROLLBACK (ignoring rollback errors) and return the commit error with context.

Kept the simple symmetric form rather than an RAII guard — lbug transactions are plain query("BEGIN"/"COMMIT"/"ROLLBACK") calls with no transaction handle type, so a guard would add indirection without much benefit at this scale.

Tests

Full suite (79 tests), cargo fmt --check, and cargo clippy --all-targets all green. The commit-success path is covered by the existing ladybug_link_edges_batch_roundtrips smoke test; the commit-failure branch isn't reliably triggerable without faking a backend error, so no brittle test was added for it.

Bumps version to 0.1.5.

The batched edge writer rolled back on a mid-batch execution error but not when
COMMIT itself failed — it just returned the error, leaving the transaction open
and risking a dangling transaction/lock on the connection. Make the COMMIT path
symmetric with the mid-batch path: on commit failure, best-effort ROLLBACK then
return the error with context. Bumps version to 0.1.5.
@prom3theu5 prom3theu5 merged commit d15fdc6 into main Jun 1, 2026
1 check passed
@prom3theu5 prom3theu5 deleted the fix/commit-rollback branch June 1, 2026 21:26
@qodo-code-review

Copy link
Copy Markdown

Review Summary by Qodo

Roll back on COMMIT failure in batched link_edges

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Add rollback on COMMIT failure in batched link_edges
• Prevent dangling transactions/locks on commit errors
• Make error handling symmetric for mid-batch and commit failures
• Bump version to 0.1.5
Diagram
flowchart LR
  A["Batch execution"] --> B{"Result OK?"}
  B -->|Yes| C["Execute COMMIT"]
  B -->|No| D["ROLLBACK + return error"]
  C --> E{"COMMIT success?"}
  E -->|Yes| F["Return OK"]
  E -->|No| G["ROLLBACK + return error"]
  D --> H["Error handled"]
  G --> H
  F --> I["Transaction complete"]

Loading

Grey Divider

File Changes

1. src/graph/ladybug_store.rs 🐞 Bug fix +10/-4

Add rollback on COMMIT failure handling

• Enhanced COMMIT error handling to perform best-effort ROLLBACK before returning error
• Added explicit match statement to handle COMMIT success and failure paths separately
• Added explanatory comment clarifying the symmetric error handling approach
• Ensures dangling transactions/locks are prevented on commit failures

src/graph/ladybug_store.rs


2. Cargo.toml ⚙️ Configuration changes +1/-1

Version bump to 0.1.5

• Bumped version from 0.1.4 to 0.1.5

Cargo.toml


Grey Divider

Qodo Logo

@qodo-code-review

qodo-code-review Bot commented Jun 1, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1)

Grey Divider


Remediation recommended

1. Unrelated Cargo.toml version bump 📘 Rule violation ⚙ Maintainability Rule 3
Description
This PR changes the crate version from 0.1.4 to 0.1.5, which is not required to implement COMMIT
rollback behavior and increases review scope. If a release bump is intended, it should be done in a
dedicated versioning/release PR or explicitly justified as part of this change.
Code

Cargo.toml[3]

Evidence
Rule 2 requires surgical changes only; the diff shows a version bump in Cargo.toml that does not
contribute to the COMMIT rollback logic change, expanding the PR beyond the minimal necessary
modifications.

Rule Rule 3: Make Surgical Changes Only (Avoid Unnecessary Refactors, Formatting, or Adjacent Cleanup)
Cargo.toml[1-4]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR includes a crate version bump in `Cargo.toml` that appears unrelated to the functional fix.

## Issue Context
The stated goal is to roll back the transaction on `COMMIT` failure in `LadybugGraphStore::link_edges`. Version bumps broaden scope and can be handled separately unless the project’s release process explicitly requires it for this fix.

## Fix Focus Areas
- Cargo.toml[3-3]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

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