CROSSLINK-302 on closing action, requester calls DeleteItem

[adamdickmeiss]

No description provided.

[Copilot]

🟡 Changes recommended

Cleanup state inference can trigger unsafe duplicate or erroneous LMS item deletions.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds requester LMS item cleanup when terminating borrowing requests.

Changes:

• Deletes temporary requester items during termination.
• Reuses requester LMS adapter setup.
• Adds cleanup success and failure tests.

File summaries

File	Description
broker/patron_request/service/action.go	Implements termination cleanup and adapter helper.
broker/patron_request/service/action_test.go	Tests requester item deletion behavior.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Receive retries and changing LMS configuration can duplicate, wrongly delete, or leak requester items.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

broker/patron_request/service/action.go:1015

• Successful accepts are persisted before the entire receive action succeeds. If a later item or the ISO message fails, the request remains retryable in SHIPPED, but the retry calls AcceptItem again for records already marked true, which can duplicate the LMS item or repeatedly fail before reaching the remaining items. Skip persisted items at the start of this loop.

		err = a.prRepo.SetRequesterLmsItemCreated(ctx, pr_db.SetRequesterLmsItemCreatedParams{

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Receive and ship-return retries can repeat LMS operations despite the new persisted marker.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Existing in-flight LMS items are not backfilled, and failed marker persistence can make ship-return retries unrecoverable.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

broker/patron_request/service/action.go:1094

• If DeleteItem succeeds but this database write fails, the action returns an error while the flag remains true. Retrying ship-return then calls NCIP DeleteItem again; an already-missing-item problem is treated as an error by the NCIP client, so the request can become permanently unable to transition. Model deletion as an idempotent/reconcilable operation (for example, treat an LMS “not found” result as success or persist a deletion workflow that can resume at the marker update).

		err = a.prRepo.SetRequesterLmsItemCreated(ctx, pr_db.SetRequesterLmsItemCreatedParams{
			ID:                      item.ID,
			RequesterLmsItemCreated: false,
		})
		if err != nil {
			status, result := logActionErrorAndReturnResult(ctx, "failed to record requester LMS item deletion", err)

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Cleanup failures can orphan LMS items, while failed compensation can leave created items permanently untracked.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

broker/patron_request/service/action.go:1024

• If both recording the successful AcceptItem and this compensating DeleteItem fail, the persisted flag remains false even though the LMS item still exists. Both ship-return and terminate then skip that item, so returning the joined error provides no recovery path and the temporary LMS item can be leaked. Persist an explicit pending/uncertain operation state and reconcile it, rather than representing this outcome as “not created.”

			if deleteErr := lmsAdapter.DeleteItem(itemId); deleteErr != nil {
				err = errors.Join(err, fmt.Errorf("LMS DeleteItem compensation failed: %w", deleteErr))

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Database failures after successful LMS deletion can make cleanup retries permanently fail.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

broker/patron_request/service/action.go:1094

• A successful LMS deletion followed by a failure to persist false leaves the marker true. The next ship-return retry reissues DeleteItem; if the LMS reports the now-missing item as an NCIP problem, this action can never advance to send the ShippedReturn message. Make deletion retries idempotent or persist/reconcile an intermediate deletion state.

		err = a.prRepo.SetRequesterLmsItemCreated(ctx, pr_db.SetRequesterLmsItemCreatedParams{
			ID:                      item.ID,
			RequesterLmsItemCreated: false,
		})
		if err != nil {
			status, result := logActionErrorAndReturnResult(ctx, "failed to record requester LMS item deletion", err)
			return actionExecutionResult{status: status, result: result, pr: pr}

• Files reviewed: 10/10 changed files
• Comments generated: 2
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

It coordinates persistent state with external LMS side effects and requires final human validation.

Review details

Suppressed comments (1)

broker/patron_request/service/action_test.go:1449

• This assertion also passes if the deletion marker was never persisted, since an absent map entry reads as false. Assert that the key exists before checking its value so a regression that omits the repository update fails this test.

	assert.False(t, mockPrRepo.requesterLmsItemCreated["item-created"])

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Cleanup failures can strand LMS items or wedge subsequent ship-return retries.

Review details

Suppressed comments (2)

broker/patron_request/service/action.go:357

• A transient DeleteItem failure is only attached as a child error, after which termination still moves the request to a terminal state. Since terminal requests are rejected by handleTerminateAction, the flagged LMS item can never be retried automatically and remains stranded. Keep the request retryable/attention-required when cleanup fails, or enqueue a durable cleanup retry before closing it.

		if err := a.deleteRequesterItemsOnClose(ctx, event.ID, pr); err != nil {
			message := "requester item cleanup failed: " + err.Error()
			closingActionError = &message

broker/patron_request/service/action.go:1093

• DeleteItem has already succeeded when this persistence step can fail, leaving the flag true. Retrying ship-return then calls NCIP DeleteItem again; an LMS that reports the now-missing item as a problem prevents the flag from ever being cleared and wedges the action. Add durable operation state/reconciliation, or explicitly make an “item not found” delete response count as successful before relying on this flag for retries.

		err = a.prRepo.SetRequesterLmsItemCreated(ctx, pr_db.SetRequesterLmsItemCreatedParams{
			ID:                      item.ID,
			RequesterLmsItemCreated: false,
		})
		if err != nil {

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟢 Approval recommended

The persistence, compensation, retry, migration, and cleanup paths are consistently implemented and adequately tested.

Review details

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Balanced