You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
src/orb/apr-repo-transfer.ts's probeAprRepoTransfer doc comment states: "anything else... is treated as still pending so the next poll retries. Never throws."
Neither createInstallationToken(env, ...) nor timeoutFetch(...) is wrapped in try/catch — both can reject (a token-mint failure, a network error, an AbortSignal.timeout abort). test/unit/orb-apr-repo-transfer.test.ts (~line 290-337) only exercises HTTP-response-level cases (404/500/malformed-JSON body); none mocks createInstallationToken rejecting or the fetch itself throwing, so the "Never throws" guarantee is currently unverified and, on inspection of the code, false.
This compounds in the caller, pollPendingAprRepoTransfers (same file, ~line 279-303): its for loop calls deps.probe(env, transfer) for each pending transfer with no per-item try/catch — unlike retryFailedRelays in src/orb/relay.ts, which processes each row independently so one bad row can't starve the rest. If probe throws for one pending transfer, the entire poll pass aborts and every other pending transfer in that batch is skipped for that tick (their pause state never re-asserted or checked). This is currently dormant — loadPendingAprRepoTransfers always returns [] until #7664 lands persistence — but the bug will start mattering the moment that lands, and fixing it now (while it's easy to test in isolation via the already-injectable AprRepoTransferPollDeps seam) is cheaper than after real pending rows exist in production.
Requirements
Wrap probeAprRepoTransfer's createInstallationToken/timeoutFetch calls in a try/catch that returns { state: "pending" } on any thrown error, making the function's own doc comment ("Never throws") actually true.
In pollPendingAprRepoTransfers, wrap each transfer's deps.probe(env, transfer) call (and the rest of that transfer's per-item work in the loop body) in a try/catch so a single transfer's failure does not abort processing of the remaining transfers in the same poll pass. On a caught error for one transfer, log it (following this file's existing logging conventions) and continue to the next transfer rather than including it in results as a false outcome.
Deliverables
probeAprRepoTransfer catches errors from createInstallationToken/timeoutFetch and returns { state: "pending" }, matching its doc comment.
pollPendingAprRepoTransfers's loop isolates each transfer's failure so one bad probe/dependency call does not prevent the rest of the batch from being processed.
A test using the existing AprRepoTransferPollDeps injection seam where deps.probe rejects for one transfer in a batch of two+, asserting the other transfer(s) are still processed normally (their results entries present with correct outcomes).
A test for probeAprRepoTransfer where the injected token-mint or fetch dependency rejects, asserting the function resolves to { state: "pending" } rather than throwing.
Test Coverage Requirements
This repo's Codecov patch gate requires 99%+ coverage of changed lines and branches. Cover both the new catch branches and confirm existing HTTP-response-level branches (404, non-ok, malformed JSON) remain covered.
Expected Outcome
probeAprRepoTransfer's "Never throws" contract is actually enforced, and a single failing probe in pollPendingAprRepoTransfers no longer silently drops every other pending transfer in the same poll pass — both now match their doc comments and are proven by tests using the module's already-injectable dependency seams.
Links & Resources
src/orb/apr-repo-transfer.ts — probeAprRepoTransfer (~line 207-223) and pollPendingAprRepoTransfers (~line 279-303)
src/orb/relay.ts — retryFailedRelays's per-row isolation pattern to mirror for pollPendingAprRepoTransfers
Context
src/orb/apr-repo-transfer.ts'sprobeAprRepoTransferdoc comment states: "anything else... is treated as still pending so the next poll retries. Never throws."Neither
createInstallationToken(env, ...)nortimeoutFetch(...)is wrapped in try/catch — both can reject (a token-mint failure, a network error, anAbortSignal.timeoutabort).test/unit/orb-apr-repo-transfer.test.ts(~line 290-337) only exercises HTTP-response-level cases (404/500/malformed-JSON body); none mockscreateInstallationTokenrejecting or the fetch itself throwing, so the "Never throws" guarantee is currently unverified and, on inspection of the code, false.This compounds in the caller,
pollPendingAprRepoTransfers(same file, ~line 279-303): itsforloop callsdeps.probe(env, transfer)for each pending transfer with no per-item try/catch — unlikeretryFailedRelaysinsrc/orb/relay.ts, which processes each row independently so one bad row can't starve the rest. Ifprobethrows for one pending transfer, the entire poll pass aborts and every other pending transfer in that batch is skipped for that tick (their pause state never re-asserted or checked). This is currently dormant —loadPendingAprRepoTransfersalways returns[]until #7664 lands persistence — but the bug will start mattering the moment that lands, and fixing it now (while it's easy to test in isolation via the already-injectableAprRepoTransferPollDepsseam) is cheaper than after real pending rows exist in production.Requirements
probeAprRepoTransfer'screateInstallationToken/timeoutFetchcalls in a try/catch that returns{ state: "pending" }on any thrown error, making the function's own doc comment ("Never throws") actually true.pollPendingAprRepoTransfers, wrap each transfer'sdeps.probe(env, transfer)call (and the rest of that transfer's per-item work in the loop body) in a try/catch so a single transfer's failure does not abort processing of the remaining transfers in the same poll pass. On a caught error for one transfer, log it (following this file's existing logging conventions) and continue to the next transfer rather than including it inresultsas a false outcome.Deliverables
probeAprRepoTransfercatches errors fromcreateInstallationToken/timeoutFetchand returns{ state: "pending" }, matching its doc comment.pollPendingAprRepoTransfers's loop isolates each transfer's failure so one bad probe/dependency call does not prevent the rest of the batch from being processed.AprRepoTransferPollDepsinjection seam wheredeps.proberejects for one transfer in a batch of two+, asserting the other transfer(s) are still processed normally (theirresultsentries present with correct outcomes).probeAprRepoTransferwhere the injected token-mint or fetch dependency rejects, asserting the function resolves to{ state: "pending" }rather than throwing.Test Coverage Requirements
This repo's Codecov patch gate requires 99%+ coverage of changed lines and branches. Cover both the new catch branches and confirm existing HTTP-response-level branches (404, non-ok, malformed JSON) remain covered.
Expected Outcome
probeAprRepoTransfer's "Never throws" contract is actually enforced, and a single failing probe inpollPendingAprRepoTransfersno longer silently drops every other pending transfer in the same poll pass — both now match their doc comments and are proven by tests using the module's already-injectable dependency seams.Links & Resources
src/orb/apr-repo-transfer.ts—probeAprRepoTransfer(~line 207-223) andpollPendingAprRepoTransfers(~line 279-303)src/orb/relay.ts—retryFailedRelays's per-row isolation pattern to mirror forpollPendingAprRepoTransferstest/unit/orb-apr-repo-transfer.test.ts— existing coverage, ~line 290-337