feat(deposition): retry assembly creation failures due to FTP login e…

[anna-parker]

…rror

resolves #

Screenshot

PR Checklist

• All necessary documentation has been adapted.
• The implemented feature is covered by appropriate, automated tests.
• Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

[claude]

Claude finished @anna-parker's task in 2m 37s —— View job

---

Code Review

• Gather context and read changed files
• Review code quality, error handling, and logic
• Check for security, performance, and testing concerns
• Post inline comments and final summary

---

The PR is a clean, focused change. The rename from trigger_retry_if_exists → retry_failed_submissions_for_matching_errors is a clear improvement in naming, and generalising the parameter from a single error_substring to error_substrings is the right design to support the new FTP login error case.

Key observations:

• No tests. There are no unit tests for retry_failed_submissions_for_matching_errors (old or new). The checklist item is unchecked — the new any()-based matching logic is simple but worth covering, especially to guard against the Iterable[str] footgun noted in the inline comment.
• Behaviour is backward-compatible for create_project.py and create_sample.py: they don't pass error_substrings, so they still use the ("does not exist in ENA",) default.
• Only assembly retries on the FTP login error, which is correct — project and sample creation use HTTP (not webin-cli/FTP), so they shouldn't be affected by this class of error.

Inline comments left on:

1. ena_submission_helper.py:886 — Iterable[str] type safety (passing a bare str would silently match characters)
2. create_assembly.py:763–767 — trailing : in the auth error substring worth confirming against a real ENA error output
3. create_assembly.py:761 — no max retry cap for persistent auth failures (pre-existing gap, but worth noting)

[claude]

The type Iterable[str] is a subtle footgun here: a plain str is also Iterable[str], so passing "some error" accidentally instead of ("some error",) would silently iterate over individual characters. Consider narrowing to Sequence[str] or tuple[str, ...] to make the interface safer.

[claude]

The first substring ends with "user error.:" — the trailing colon after the full stop looks unusual. Is this literally what ENA returns in the error message? If the colon is part of the actual message (e.g. preceding a stack trace), the match is fine, but it's worth confirming against a real example so the substring doesn't silently stop matching if ENA changes formatting.

[claude]

The FTP auth retry will loop indefinitely for persistent credential failures — only the retry_threshold_min cooldown limits frequency, but there's no max attempt count. This pattern already exists for the "does not exist in ENA" case, so it's not a new regression, but it means a persistent misconfiguration will keep retrying forever without alerting. The TODO on line 774 acknowledges this gap.