Skip to content

CROSSLINK-264 retry-action#622

Open
adamdickmeiss wants to merge 51 commits into
mainfrom
CROSSLINK-264-ask-retry-action
Open

CROSSLINK-264 retry-action#622
adamdickmeiss wants to merge 51 commits into
mainfrom
CROSSLINK-264-ask-retry-action

Conversation

@adamdickmeiss

Copy link
Copy Markdown
Contributor

Copilot AI review requested due to automatic review settings May 29, 2026 11:58
@adamdickmeiss adamdickmeiss marked this pull request as draft May 29, 2026 11:58

Copilot AI 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.

Pull request overview

This PR adds retry-related entries to the returnables patron-request state model for CROSSLINK-264, introducing a requester-side retry event and supplier-side retry action.

Changes:

  • Adds accept-retry event on requester SENT, transitioning to NEW.
  • Adds ask-retry action on supplier VALIDATED, transitioning to NEW.
  • Keeps generated runtime JSON in sync with misc/returnables.yaml.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
misc/returnables.yaml Adds retry event/action declarations to the source state model.
broker/patron_request/service/statemodels/returnables.json Updates the generated embedded runtime state model with the same retry declarations.

Comment thread misc/returnables.yaml Outdated
Comment thread misc/returnables.yaml Outdated
Copilot AI review requested due to automatic review settings June 1, 2026 12:31

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Comment thread misc/returnables.yaml
Comment thread misc/returnables.yaml
Comment thread broker/patron_request/service/statemodels/returnables.json
Comment thread broker/patron_request/service/statemodels/returnables.json
Comment thread misc/returnables.yaml
Comment thread broker/patron_request/service/statemodel_capabilities.go
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 1, 2026 12:38
adamdickmeiss and others added 2 commits June 1, 2026 14:38
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI 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.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

Comment thread broker/patron_request/service/statemodels/returnables.json
Comment thread misc/returnables.yaml
Comment thread misc/returnables.yaml
Comment thread misc/returnables.yaml
Comment thread broker/patron_request/service/statemodel_capabilities.go
Comment thread broker/patron_request/service/statemodels/returnables.json
Copilot AI review requested due to automatic review settings June 2, 2026 13:02

Copilot AI 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.

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Comment thread broker/patron_request/service/statemodel_capabilities.go
Comment thread broker/patron_request/service/message-handler.go
Comment thread misc/returnables.yaml Outdated

Copilot AI 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.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Comment thread broker/patron_request/service/action.go Outdated
Copilot AI review requested due to automatic review settings June 19, 2026 11:18

Copilot AI 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.

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.

Comment thread broker/patron_request/service/action.go Outdated
Copilot AI review requested due to automatic review settings June 19, 2026 11:44

Copilot AI 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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Comment thread broker/patron_request/service/action.go Outdated
Comment thread broker/patron_request/service/message-handler.go Outdated
Copilot AI review requested due to automatic review settings June 19, 2026 11:55
@adamdickmeiss adamdickmeiss requested a review from jakub-id June 19, 2026 12:00

Copilot AI 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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.

Comment thread broker/test/patron_request/api/api-handler_test.go Outdated
Comment thread broker/test/patron_request/api/api-handler_test.go Outdated
Comment thread broker/test/patron_request/api/api-handler_test.go Outdated
@jakub-id

Copy link
Copy Markdown
Contributor

broker/patron_request/service/action.gofinalizeActionExecution, retry PR auto-action error handling

When auto-actions fail on the new (retry) PR, there is no trace of this failure on the original PR. I think we should ensure the original PR is also marked as needing attention (NeedsAttention = true) in this case — similar to what we do when auto-actions fail on the original PR itself (where markActionChainFailure is called explicitly). As it stands, the original PR ends up in RETRY_ACCEPTED with no indication that something went wrong with the follow-up.

@jakub-id

Copy link
Copy Markdown
Contributor

misc/returnables.yaml / broker/patron_request/service/statemodels/returnables.json — supplier WILL_SUPPLY state

ask-retry should also be available from WILL_SUPPLY on the supplier side, not only from VALIDATED. A supplier might discover that the item is not found as cited after they have already indicated they will supply (e.g. during physical retrieval). Without this, a lender in WILL_SUPPLY state has no way to request a retry.

Copilot AI review requested due to automatic review settings June 19, 2026 16:01

Copilot AI 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.

Pull request overview

Copilot reviewed 33 out of 33 changed files in this pull request and generated no new comments.

@adamdickmeiss

Copy link
Copy Markdown
Contributor Author

broker/patron_request/service/action.gofinalizeActionExecution, retry PR auto-action error handling

When auto-actions fail on the new (retry) PR, there is no trace of this failure on the original PR. I think we should ensure the original PR is also marked as needing attention (NeedsAttention = true) in this case — similar to what we do when auto-actions fail on the original PR itself (where markActionChainFailure is called explicitly). As it stands, the original PR ends up in RETRY_ACCEPTED with no indication that something went wrong with the follow-up.

Done

@adamdickmeiss

Copy link
Copy Markdown
Contributor Author

misc/returnables.yaml / broker/patron_request/service/statemodels/returnables.json — supplier WILL_SUPPLY state

ask-retry should also be available from WILL_SUPPLY on the supplier side, not only from VALIDATED. A supplier might discover that the item is not found as cited after they have already indicated they will supply (e.g. during physical retrieval). Without this, a lender in WILL_SUPPLY state has no way to request a retry.

Done

@jakub-id jakub-id 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.

@adamdickmeiss There's a conceptual problem here: the NotFoundAsCited aka Transfer must restart locating suppliers because the new Bib ID can result in different holdings. This is a different use case than the regular retry which is sent to the same supplier and the rota is unchanged. Since we operate on the same transaction, I think we should call locate-suppliers again but change it so that it skips all existing suppliers first, does the new lookup and appends new suppliers to the list. Then select the first new supplier. It also means the fix for supplierUniqueId overriding the localId is not be needed -- we should only use the updated ID when the reason retry is NotFoundAsCited or Transfer.

Comment thread broker/oapi/open-api.yaml
prevReqId:
type: string
description: ID of the previous patron request in the sequence
retryBibInfo:

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.

this should use a proper Go type in the API model via the overlay and avoid going through the map

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants