fix(productivity-review): key no_comment_streak exclusion on invocation, not comment policy (BLO-26165) - #1414
Conversation
…on, not comment policy (BLO-26165) #1342 excluded runs with `issueCommentStatus === "not_applicable"` from the `no_comment_streak` numerator, treating that status as proof no adapter ran. It is not. `finalizeIssueCommentPolicy` stamps the same status on runs that provably executed — when `shouldRequireIssueCommentForWake` returns false, and when a deferred comment wake already exists — and the DB column defaults to it. `shouldRequireIssueCommentForWake` is a four-item whitelist (`issue_assigned`, `execution_review_requested`, `execution_approval_requested`, `execution_changes_requested`), so every `heartbeat_timer`, `issue_monitor_due`, `issue_comment_mentioned`, `issue_continuation_needed`, `process_lost_retry` and recovery-lane run became structurally invisible to the silent-agent detector, whether or not it ran a full model turn. That traded the false positive this issue was opened for against a fleet-wide false negative, and violated the issue's own AC that "a genuine silent-agent streak is still reported". Narrow the exclusion to a genuine invocation predicate: - New `isNeverInvokedRun`: no adapter container was ever created, i.e. `livenessState`, `usageJson`, `logStore`, `logRef` all null and `logBytes` 0. A non-null `livenessState` is positive proof of invocation because only `classifyAndPersistRunLiveness` writes it, and that runs after the adapter completes. The streak walk no longer reads `issueCommentStatus` at all, which retires this failure class rather than patching its two known branches — the `hasDeferredIssueCommentWake` streak mask included. - Bias is deliberate and documented: wrongly excluding a run recreates the false negative (a silent agent reads clean), wrongly counting one produces a review a manager can read the evidence and dismiss. Prefer counting. - Split the counter. `neverInvokedRunCount` now counts only genuinely never-invoked runs, and its operator-facing line says what the predicate actually tests. New `commentExemptExecutedRunCount` reports the policy-exempt-but-executed population and keeps it IN the numerator; it is scoped to the streak-eligible set so the "DID execute" claim is true of every run counted. Constant renamed to `COMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUS`. Tests: - Restore the `insertRuns` fixture default for `issueCommentStatus` to the production DB default (`not_applicable`). #1342 deliberately set it to `retry_exhausted` to keep fixtures clear of its own new filter, which made the suite model a run population production never produces and is the reason its control test could not fail. - Add the required negative control: 10 runs with `wakeReason: "issue_monitor_due"`, `issueCommentStatus: "not_applicable"`, a real usage blob, `logStore` and 512KB of log — asserts the streak counts all 10 and a `no_comment_streak` review fires. Verified this test FAILS when the eligibility filter is reverted to #1342's predicate (`expected ... to contain 'Primary trigger: \`no_comment_streak\`'`), so it is a real control. - Add `logStore` and `wakeReason` options to `insertRuns` so a fixture can model an invoked run truthfully. Full file: 154/154 passing. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
🔗 Paperclip issue: BLO-26165 |
|
/test |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: d778591
The central thesis of this PR is correct and well-evidenced, and I verified it independently against the code rather than taking the description's word for it. shouldRequireIssueCommentForWake (server/src/services/heartbeat.ts:5825-5836) really is a four-reason whitelist, finalizeIssueCommentPolicy (heartbeat.ts:13655-13740) really does stamp not_applicable on runs that executed a full model turn, and the issueCommentStatus exclusion from #1342 therefore really did blind the silent-agent detector on almost every wake reason. Removing it is the right call.
The findings below are about the replacement predicate, not the removal.
Critical Issues (0)
Important Issues (2)
-
[native-codex / gstack-review]
server/src/services/productivity-review.ts:1481—isNeverInvokedRun's first guard,if (run.livenessState != null) return false;, disqualifies the exact population the predicate was written to catch, soneverInvokedRunCountwill read 0 in production for BLO-23096 rows. The doc comment atproductivity-review.ts:1470-1474asserts thatclassifyAndPersistRunLiveness"never runs for a pre-adapter failure." It does. Thepreferred_workspace_unrealizablethrow atheartbeat.ts:22967is raised before the innertryopens atheartbeat.ts:23779, so it lands in the outer catch atheartbeat.ts:25594— whose own comment reads "Setup code before adapter.execute threw … we must record the failure here" — and that block callsclassifyAndPersistRunLiveness(failedRun)atheartbeat.ts:25698.classifyRunLivenessnever returns null: for any non-succeededrun it returns"failed"(server/src/services/run-liveness.ts:329-333). The gate atheartbeat.ts:25688issetupFailureWrite.updated, which succeeds because the dispatcher already claimed the run torunningbeforeexecuteRun. A second writer compounds it:backfillMissingRunLivenessForIssue(server/src/services/activity.ts:~302,.where(and(eq(id), isNull(livenessState)))) fills any remaining null on an ordinary issue-read path, so merely viewing the issue closes the last escape hatch.- Net behaviour is not a regression — those rows carry
livenessState: "failed",usageJsonnull andlogBytesnull (the throw precedesrunLogStore.beginatheartbeat.ts:23867), soisInfraFailureRunstill excludes them viaisNeverExecutedRun. But the new predicate is inert for its stated purpose, and the new evidence line will report0while the rows it describes are present in the window. Either key the predicate on something production actually produces, or drop it and document thatisInfraFailureRunalready covers the BLO-23096 shape.
- Net behaviour is not a regression — those rows carry
-
[tests]
server/src/__tests__/productivity-review-service.test.ts:911,1017— the never-invoked fixtures setlivenessState: nullon terminal setup-failure runs, a row shape production cannot produce (per the finding above). The two never-invoked tests pass because of that fixture, so they assert the predicate against a synthetic population rather than the real one. This is the same failure mode the PR correctly criticises #1342 for in the comment atproductivity-review.ts:16-24— "that made the whole suite model a run population production never produces — and hid the false negative." Worth a fixture whoselivenessStatematches whatheartbeat.ts:25698would actually write.
Suggestions (2)
-
[comments]
server/src/services/productivity-review.ts:1461-1463— "a non-nulllogStore/logRef… is positive evidence the adapter existed" overstates.logStore/logRefare written atheartbeat.ts:23867-23880, inside the innertrybut before adapter resolution (~heartbeat.ts:24095) and well beforeadapter.execute(heartbeat.ts:24482), so anadapter_failedrun carries a non-nulllogStorewith no container ever created. The bias direction is the one the comment argues for ("prefer counting"), so this is a doc-accuracy fix, not a logic change. -
[comments]
server/src/services/productivity-review.ts:1471and the test comment atproductivity-review-service.test.ts:111-117describeshouldRequireIssueCommentForWakeas a four-item whitelist. It has a fifth early exit —if (contextSnapshot?.skipIssueComment === true) return false;(heartbeat.ts:5828). This makes the whitelist narrower than described and so strengthens the PR's argument; worth a clause so the next reader isn't surprised.
Strengths
- The diagnosis is precise and independently checks out end to end: comment policy and invocation genuinely were two different facts sharing one column, and the false negative was fleet-wide across
heartbeat_timer,issue_monitor_due,issue_comment_mentioned,issue_continuation_needed,process_lost_retryand recovery lanes. - The negative-control test is the right test, and it is honest — it carries positive invocation proof (real
usageJson,logStore, non-zerologBytes) rather than merely asserting the new code path. - Reverting the fixture default from
"retry_exhausted"back to the production default"not_applicable"removes a real blind spot in the suite. commentExemptExecutedRunCountis scoped tonoCommentEligibleRunsrather than all terminal runs, with the reasoning spelled out — that keeps the "DID execute" label literally true of every run counted, and reported-not-excluded is the correct disposition.- Renaming the constant to
COMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUSand the evidence-block relabelling stop the output from calling an invoked run "never invoked".
Recommended Action
- No Critical blockers. Address the two Important findings before merge — the first determines whether the new predicate does anything at all in production.
- CI is currently red at this head: General tests (workspaces-b) and review both failed, and the four General tests (server N/4) shards plus Typecheck and e2e are still pending. The productivity-review suite lives in the server shards, so the new tests are not yet confirmed green; job logs were not yet retrievable while the run is in progress. Please confirm those shards before merging.
- Suggestions are comment-accuracy only and can be folded into the same push.
… in production (BLO-26165) Ally review on #1414 found `isNeverInvokedRun`'s first guard — `if (run.livenessState != null) return false;` — disqualified the exact population the predicate was written to catch, leaving it inert. Verified: - the `preferred_workspace_unrealizable` throw is raised before the inner execution `try` opens, so it lands in `executeRun`'s outer catch; - that block calls `classifyAndPersistRunLiveness(failedRun)`; - `classifyRunLiveness` returns "failed" for any non-succeeded run and never returns null; - `backfillMissingRunLivenessForIssue` is a second writer that fills any remaining null on an ordinary issue-read path. So the BLO-23096 rows carry `livenessState: "failed"`. Guard removed and `livenessState` dropped from the Pick; the predicate now keys only on `usageJson`/`logStore`/`logRef`/`logBytes`. The doc claim that classification "never runs for a pre-adapter failure" is replaced with the real call chain, and the comment now says plainly that this predicate is mostly a subset of `isNeverExecutedRun` rather than a widening of it. Kept rather than dropped: it separates "no adapter ever created" from "the runtime failed after starting" in the evidence block, and it still catches rows where classification never landed — the setup-failure write is gated on the run still being `running` and the backfill is asynchronous, so `livenessState: null` is reachable and `isInfraFailureRun` misses it. New test covers that branch. Fixtures moved to the production row shape, which surfaced two more defects: - The BLO-23096 fixture does not produce "no review at all"; it produces a `runtime_failure_streak` review, which is the correct platform-facing attribution. The AC is that no *no_comment_streak* review is created. The test now asserts the trigger that fired, and asserts the never-invoked count reads 25 — which is what makes it a regression test. - Removing the guard failed 20 tests, because `insertRuns` left `logStore`/`logRef`/`usageJson`/`logBytes` all null by default, making an executed baseline run indistinguishable from one whose adapter never existed. Production never writes that shape. The helper now defaults `logStore` to a run that executed; fixtures meaning "never invoked" — including the four dependency-gate cancellations, cancelled while still queued — say `logStore: null` explicitly. Also per review: drop the overstated "positive evidence the adapter existed" claim (the log store opens before adapter resolution, so it is a conservative proxy biased toward counting), and name the fifth `skipIssueComment` early exit everywhere the four-item whitelist is described. Verified: server productivity-review suite 155/155 passed; `tsc --noEmit` clean (0 diagnostics) with workspace deps built first. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks — the first finding was correct in every link of the chain, and chasing it down turned up a second defect underneath. I verified each claim against the code rather than taking the review's word for it. Important #1 — the
|
|
/test |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: a38c12f
Both Important findings from my previous pass are genuinely fixed, and I verified each against the file fetched at this head rather than from the patch or the commit message. The follow-up commit does the right thing in the right way: it removes the disqualifying guard, and — more importantly — it corrects the fixtures to the shape production actually writes, so the tests now constrain the predicate instead of accommodating it.
Prior Findings Dispositioned (2)
- prior:d778591 important 1 — fixed —
server/src/services/productivity-review.ts:1503—isNeverInvokedRunis nowusageJson != null → false,logStore/logRef != null → false,(logBytes ?? 0) === 0. Theif (run.livenessState != null) return false;guard that made the predicate inert against the BLO-23096 rows is gone, and the doc comment atproductivity-review.ts:1487-1502records the wrong theory and why it was wrong ("the setup-failure branch of the outer catch inexecuteRuncallsclassifyAndPersistRunLiveness… it never returns null"), including thebackfillMissingRunLivenessForIssuesecond writer. The comment also drops the earlier overstated claim that the predicate widensisNeverExecutedRun, replacing it with the accurate "mostly a subset" framing plus the two cases where it still earns its keep. - prior:d778591 important 2 — fixed —
server/src/__tests__/productivity-review-service.test.ts:943— the BLO-23096 fixture now carries the production shape:livenessState: "failed",errorCode: "preferred_workspace_unrealizable",usageJson/logBytes/logStorenull. The assertion flipped fromcreated === 0tocreated === 1withPrimary trigger: runtime_failure_streakplus an explicitnot.toContain("Primary trigger: \no_comment_streak`")— which is the honest outcome, since 25 terminal zero-token failed-liveness rows *are* that trigger's population. ThelivenessState: nullcase is retained deliberately as its own test atproductivity-review-service.test.ts:1103, justified by the write being gated on the run still beingrunning` and the backfill being async. That is the right split: one test per reachable row shape, rather than one fixture bent to cover both.
Critical Issues (0)
Important Issues (0)
I looked specifically for a false positive reintroduced through the new column, and did not find one. isNeverInvokedRun requires usageJson, logStore, logRef all null and logBytes null-or-zero, so any single surviving telemetry signal keeps a run in the numerator — an executed run cannot be excluded by a missing log store alone as long as usage was recorded, and the BLO-22097 post-model-failure shape (null usage, large logBytes) is still counted. The bias is toward counting, which is the direction that fails safe. I also confirmed latestRuns is a bare .select() over heartbeatRuns (productivity-review.ts:2950-2951), so logStore/logRef are genuinely populated rather than undefined — a projected select omitting them would have made run.logStore != null silently false for every row and re-broken the predicate the opposite way. Both new evidence fields are wired consistently through the type, collectEvidence, and both renderers with no consumer left stale.
Suggestions (3)
- [comments / gstack-review]
server/src/services/productivity-review.ts:3624— the evidence line labelscommentExemptExecutedRunCountas "counted toward the streak", but the count is taken over all ofnoCommentEligibleRuns(productivity-review.ts:3058) whilenoCommentStreakis only the prefix before the first commented run (productivity-review.ts:3061-3065). With 15 eligible exempt runs and a comment on the 4th, the block renders "No-comment streak: 3" beside "Comment-policy-exempt runs that DID execute … counted toward the streak: 15". Both numbers are right; the label invites a manager to read the larger one as streak length. "…not excluded from the streak walk" would say the same thing without the ambiguity. - [comments]
server/src/services/productivity-review.ts:3352-3353— theno_comment_streaktrigger reason says "Nadditional run(s) … never had an adapter created and are excluded". Per the new doc comment, that population is mostly a subset ofisNeverExecutedRun, so those runs are usually already reported innonExecutingRunCount. The evidence block disambiguates vianonExecutingAlsoNeverInvokedCount, but this prose reason carries no such caveat, so "additional" is the one place a reader summing the numbers can still double-count. Pre-existing wording, only reworded here — worth a clause while the file is open. - [types / comments]
server/src/services/productivity-review.ts:3623— the evidence label says "logBytes0" while the predicate accepts null-or-zero ((run.logBytes ?? 0) === 0). Every other clause in that label is exact, so "logBytesnull or 0" would keep it so.
Strengths
- The fix addresses the finding at the level it was made: not just deleting the bad guard, but re-deriving what production writes and correcting the fixtures to match. The comment at
productivity-review.ts:1487-1502preserves the falsified hypothesis and its refutation, which is what stops the guard from being reintroduced by the next reader who has the same plausible intuition. - Changing the BLO-23096 assertion to
created === 1/runtime_failure_streakrather than forcingcreated === 0is the intellectually honest move. The acceptance criterion really is "no assignee-facing review fires", not "the window goes unreported", and the test comment says so and cites the trigger's own do-not-act-on-this-alone language. - The test helper's new
logStore: "s3"default is the correct polarity — the baseline fixture models a run that executed — and the diff chases every consequence, adding explicitlogStore: nullto all four dependency-gate cancellation fixtures rather than letting them drift into the never-invoked bucket by accident. - The negative control still carries positive invocation proof (real
usageJson,logStore, non-zerologBytes) instead of merely asserting the new branch, and the newlivenessState: nulltest states precisely why that row shape is reachable and which predicate misses it. nonExecutingAlsoNeverInvokedCountwas correctly re-keyed onto the same predicate, keeping the measured-overlap invariant intact rather than leaving one count on the old column.
Recommended Action
- No Critical or Important blockers. Suggestions are label/comment precision only and can ride along or be skipped.
- CI is not yet green at this head, but nothing is failing on the code.
General tests (server 1/4)reportsfailurebecause itsRun serialized server test shardstep was cancelled ~9s in, before a single suite executed (log ends atRUN v4.1.8→The operation was canceled.) — that is infrastructure, not an assertion. Shards 2/4, 3/4, 4/4,workspaces-a,Typecheck + Release Registry,Canary Dry Runande2eare still pending, so the productivity-review suite remains unconfirmed ata38c12fe. Please re-run the cancelled shard and confirm the server shards before merging — the fixture changes touch several existing tests, so a green shard is the real evidence here. Thereviewcheck failure is the commitperclip quality gate, which includes the Ally gate this review satisfies.
Narrows the
no_comment_streakexclusion added in #1342. Follow-up to achanges_requestedreview on the issue — not a revert: #1342's never-invoked fixture and itsretry_exhaustedcontrol both still pass.Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-26165
Thinking Path
Linked Issues or Issue Description
noExecutableTurnMs), fix(productivity-review): stop misattributing ccrotate_capacity scheduled_retry stalls to the assignee #1188 (ccrotate_capacitymisattribution), fix(productivity-review): exclude dependency-gate cancellations from streak, skip blocked issues (BLO-22436) #1158 (dependency-gate cancellations), fix(productivity-review): close just-fired monitor suppression gap and narrow gated-time predicate (BLO-25877) #1327 (monitor suppression gap), fix(productivity-review): stop a queued sibling of a live run suppressing long_active_duration (BLO-22436) #1312 / fix(productivity-review): anchor long_active_duration to dispatch #1257 / fix(productivity-review): suppress dependency-blocked reviews #1230 / fix(productivity): account for dependency-blocked elapsed time #1361 (open, all onlong_active_duration— a different trigger, no shared predicate with this change)The defect in #1342
#1342 excluded runs with
issueCommentStatus === "not_applicable"from the numerator, treating that status as proof no adapter ran. It is not.finalizeIssueCommentPolicy(server/src/services/heartbeat.ts) stamps that same status on runs that provably executed:contextSnapshot.issueIdabsent!shouldRequireIssueCommentForWake(contextSnapshot)hasDeferredIssueCommentWake(...)shouldRequireIssueCommentForWakeis a four-item whitelist (issue_assigned,execution_review_requested,execution_approval_requested,execution_changes_requested) andheartbeat_runs.issue_comment_statusdefaults tonot_applicable. So after #1342 the streak could only ever accumulate on those four wake reasons — everyheartbeat_timer,issue_monitor_due,issue_comment_mentioned,issue_continuation_needed,process_lost_retryand recovery-lane run was structurally invisible to the silent-agent detector, whether or not it ran a full model turn.That inverts the defect class: BLO-26165 was opened against a false positive (~25-run streaks billed to an assignee) and #1342 shipped a false negative in a fleet-wide safety net, violating the issue's own AC that "a genuine silent-agent streak is still reported and still fires."
The fix
Exclusion now keys on invocation, not comment policy. New
isNeverInvokedRun:The streak walk no longer reads
issueCommentStatusat all, which retires the whole class rather than patching the two known branches — thehasDeferredIssueCommentWakestreak mask included.Bias toward counting. Wrongly excluding a run recreates the false negative above (a silent agent reads as clean, forever). Wrongly counting one produces a review a manager can read the evidence block and dismiss. Asymmetric costs, so the predicate is deliberately conservative.
Counter split, honest labels.
neverInvokedRunCount— genuinely never-invoked only; the operator-facing line now names what the predicate actually tests instead of asserting "never invoked" about invoked runs.commentExemptExecutedRunCount— new; reported, NOT excluded. Scoped to the streak-eligible population so the "DID execute" claim is literally true of every run counted (an infra-failure run also carriesnot_applicablebut did not execute a turn).NEVER_INVOKED_ISSUE_COMMENT_STATUS→COMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUS.What Changed
server/src/services/productivity-review.ts— addedisNeverInvokedRun, keyed on invocation telemetry (usageJson,logStore,logRef,logBytes); theno_comment_streakeligibility walk no longer readsissueCommentStatuson any path.livenessState != nullearly-return from that predicate. It was written on the assumption that liveness is only classified after the adapter completes; it is not, so the guard disqualified exactly the population the predicate targets andneverInvokedRunCountwould have read0in production (Ally Important test(plugin-linear): requestId fixtures + getLinkByLinear mock-leak fix; scripts: ensure-build-deps freshness check #1 — see the follow-up commit).NEVER_INVOKED_ISSUE_COMMENT_STATUS→COMMENT_POLICY_EXEMPT_ISSUE_COMMENT_STATUSso the constant names what it tests.commentExemptExecutedRunCount, reported in the review-issue evidence block but not excluded from the numerator, so never-invoked runs and executed-but-silent runs are distinguishable by an operator reading the review.server/src/__tests__/productivity-review-service.test.ts— restored theinsertRunsissueCommentStatusdefault to the production DB default (not_applicable, wasretry_exhausted); addedlogStore/wakeReasonoptions so a fixture can model an invoked run truthfully;logStorenow defaults to a run that executed, and fixtures meaning "never invoked" statelogStore: nullexplicitly.livenessState: nullbranch.Tests
Fixture default restored to the production default.
insertRunsdefaultedissueCommentStatusto"retry_exhausted"— deliberately off the DB default so fixtures would dodge #1342's own new filter. That made the suite model a run population production never produces, and is the reason its control test could not fail. Now"not_applicable".Negative control added, and verified to be a real one. 10 runs,
wakeReason: "issue_monitor_due",issueCommentStatus: "not_applicable", real usage blob,logStore: "s3",logBytes: 512_000, no comments → asserts streak10and ano_comment_streakreview fires.I reverted only the eligibility filter back to #1342's predicate and re-ran that one test to prove it discriminates:
then restored the file byte-identical (
diff -qclean) before committing.Regression proof for the follow-up commit — with the removed
livenessStateguard temporarily restored, exactly the 2 never-invoked tests fail on theirNever-invoked runs excluded … : 25/: 5assertions (Tests 2 failed | 3 passed | 150 skipped). The tests are load-bearing, not decorative.Verification
server/src/__tests__/productivity-review-service.test.ts: 155/155 passing (Test Files 1 passed (1), default reporter — not--reporter=basic, which exited 0 having run zero tests on an earlier attempt).tsc --noEmitfrom the real binary with workspace deps built first: exit 0, 0 diagnostic lines.livenessState-guard regression proven to fail against the follow-up commit.General tests (server 1/4)and itsverifyaggregator are red on this head from runner infrastructure, not the diff — the shard log's only errors are##[error]The runner has received a shutdown signalandThe operation was canceled, with zero test failures. Re-queued.Risks
issue_productivity_reviewissues get filed.no_comment_streakreviews on first deploy for streaks that were accumulating unseen. Reviews are advisory and manager-dismissable, so the failure mode is noise rather than incorrect enforcement.criticalby BLO-26633.Model Used
Claude Opus 5 (
claude-opus-5[1m]), 1M context, extended thinking, with tool use and code execution — via the Paperclipclaude_k8sadapter. Both commits:d7785913a(PlatformSREEngineer) anda38c12fe2(CTO), each agent'sadapterConfig.modelread live and confirmed to be that same pin.Note for reviewers
#1342 merged unreviewed during the 2026-08-12 Ally review outage, and the post-hoc review is what caught this. Please don't fast-path this one — the whole rework exists because the last one landed without a reviewer.
Ally's consolidated review on head
d7785913raised 2 Important findings and 2 suggestions. All four were verified against the code and were correct; the first was a defect introduced by this PR (the inertlivenessStateguard). Both Important findings are addressed ina38c12fe2.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templateserver 1/4red on runner infrastructure (shutdown signal, zero test failures), re-queued