fix(productivity-review): grace window for monitor lapse-to-service gap (BLO-21003) - #985
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: 521f486
Important Issues (3)
- [code/tests]
server/src/services/productivity-review.ts:1068— The backlog rank does not match the dispatcher's ordering for equal due times. This query counts every monitor whosemonitorNextCheckAtis less than or equal to the source timestamp, whiletickDueIssueMonitorsorders ties byupdatedAt. A large equal-time cohort can therefore make even the first monitor look many batches deep and suppress a genuine productivity review far beyond its service position. Recommendation: rank only rows preceding the source under the exact scheduler ordering, add a deterministic final tie-breaker to both queries, and cover an equal-timestamp cohort where the earliest row receives first-batch grace. - [gstack/native-codex]
server/src/services/productivity-review.ts:1081— The derived grace uses the remaining queue depth but is compared with total age since the monitor became due. As earlier batches drain,duePositionand grace shrink whiledueAgeMsgrows, so a monitor still legitimately queued can become reviewable before its dispatch tick. For example, after nine minutes of a ten-batch backlog, one remaining batch yields roughly six minutes of grace and the still-pending monitor is flagged. Recommendation: compute an absolute expected service deadline or preserve elapsed queue delay when converting remaining position into grace, then test reconciliation after multiple partial batches have drained. - [gstack/native-codex]
server/src/services/productivity-review.ts:1073— A concurrent scheduler claim can still produce the false review this change targets. Candidates are read earlier; if dispatch setsmonitorWakeRequestedAtafter that snapshot, this count excludes the now-fresh source, but the second suppression check atserver/src/services/productivity-review.ts:1267still evaluates the stale candidate with a null claim. Once base grace has elapsed and no earlier rows remain, backlog grace is zero and reconciliation creates a review despite the active claim. Recommendation: re-read the source monitor state as part of the decision, or return and evaluate its current durable claim atomically; add a claim-between-candidate-read-and-backlog-count regression test.
Strengths
- The close path correctly remains stricter than new-review suppression, preventing a due-but-unserviced wake from starting the resolved-review snooze.
- Fresh claim evidence is included in suppression activity, and the long-lapsed negative control preserves genuine escalation behavior.
- The scheduler cadence, capped backlog, and sub-minute evidence formatting all receive focused regression coverage.
Recommended Action
- Fix the three Important issues before merge.
- Add equal-timestamp, partially drained backlog, and concurrent-claim regression cases.
- Re-run the focused embedded-Postgres suite and required CI checks.
|
@ally please review exact head |
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: 262b5b4
Prior Findings Dispositioned (3)
- prior:521f486 important 1 — fixed —
server/src/services/productivity-review.ts:1087— The backlog query now uses the scheduler's(monitorNextCheckAt, updatedAt, id)ordering, including a deterministicidtie-breaker that is also present intickDueIssueMonitors. - prior:521f486 important 2 — fixed —
server/src/services/productivity-review.ts:1113— Backlog grace now includes the already elapsed due age before adding remaining dispatch time, so draining earlier batches no longer shrinks the deadline below elapsed queue time. - prior:521f486 important 3 — still-present —
server/src/services/productivity-review.ts:1125— The source is re-read before backlog calculation, but a scheduler claim can still land after this read. The backlog query then excludes the freshly claimed source while the final suppression check reuses this stale, unclaimed row.
Important Issues (2)
- [prior:521f486 important 3]
server/src/services/productivity-review.ts:1125— A scheduler claim racing betweengetCurrentIssue()andmonitorBacklogGraceMs()can still create the false review this PR targets. If base grace has elapsed, the query excludes the now-claimed source, returns position zero, anddeliberatePendingMonitorevaluates the stale row with no claim. Recommendation: make the current-state read and suppression decision atomic, or re-read the source after backlog calculation before returning an unsuppressed result; add a regression hook in this exact window. - [gstack/native-codex]
server/src/services/productivity-review.ts:1102— Freshly claimed predecessor monitors disappear fromduePosition. When the source is immediately behind one in-flight predecessor, only the source remains counted,duePosition === 1disables backlog grace, and an expired base grace can file a review while the scheduler is still dispatching the preceding row. Recommendation: count fresh-claimed predecessors that sort before the source as live queue work, while handling the source's own fresh claim separately; add a test with one claimed predecessor and an old due source.
Strengths
- The backlog rank now matches scheduler ordering, including deterministic equal-time ties.
- The elapsed queue delay is preserved as earlier batches drain.
- The focused tests cover equal timestamps, partial backlog drainage, and a claim made before evidence collection.
Recommended Action
- Close both remaining claim-window races before merge.
- Add focused regressions for a source claim after the current-state read and for an in-flight predecessor.
- Run the embedded-Postgres targets in CI; the current review and security-review checks are green while policy remains pending.
|
Supplementary pass — a concurrent Ally run already submitted the operative verdict for Ally — Additional FindingReviewed head: 262b5b4 Important Issues (1)
|
|
@ally please review exact head |
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: 5bdb032
Prior Findings Dispositioned (2)
- prior:521f486 important 3 — fixed —
server/src/services/productivity-review.ts:1140— The source is now re-read after backlog calculation, andlatestDirectat line 1143 honors a claim that landed after the initial current-state read. - prior:262b5b4 important 2 — still-present —
server/src/services/productivity-review.ts:1083— Fresh-claimed equal-time predecessors are compared using their currentupdatedAt, but the scheduler rewrites that field when claiming them, so production claims can move predecessors behind the unclaimed source and remove their queue contribution.
Important Issues (3)
- [prior:262b5b4 important 2]
server/src/services/productivity-review.ts:1083— The claimed-predecessor fix does not preserve scheduler order for equal due timestamps.tickDueIssueMonitorssets a claimed row'supdatedAttonow, so each of the 50 selected predecessors can stop satisfyingupdatedAt < source.updatedAtwhile that batch is still dispatching. A source just beyond the batch boundary then appears first, loses backlog grace, and can receive a false productivity review. The regression atserver/src/__tests__/productivity-review-service.test.ts:1200seeds a fresh claim but leavesupdatedAtartificially older than the source, so it cannot expose the production race. Preserve the pre-claim ordering key, or rank claimed predecessors without relying on the mutatedupdatedAt, and make the test claim through the production update shape. - [gstack/native-codex]
server/src/services/productivity-review.ts:1124— Backlog grace is renewed indefinitely because every reconciliation returnsdueAgeMs + remainingTime. Whenever any predecessor remains,dueAgeMsis therefore always below the newly computed grace, even if a poison predecessor has failed dispatch for hours or days. This contradicts the bounded service-latency goal and hides genuinely unserviced sources forever. Anchor the deadline to stable queue evidence rather than the current time, and add a negative control with a non-draining predecessor. - [code/tests]
server/src/services/productivity-review.ts:1140— The final monitor re-read is still separated from review creation by snooze/backoff/owner queries andevaluateSourceReviewability, which does not read monitor fields. A scheduler claim or rearm after this read but beforecreateOrUpdateReviewstill creates the false review the durable-claim guard is intended to prevent. Revalidate monitor suppression in the creation critical section or make the decision conditional on unchanged monitor state; the existingbeforeCreateOrUpdateReviewhook can cover this final race window.
Strengths
- The post-backlog source re-read closes the specific claim-after-current-state-read window from the prior review.
- The open-review close path remains strict, avoiding a resolved-review snooze before monitor dispatch succeeds.
- Scheduler ordering now has a deterministic
idtie-breaker, and the focused suite covers the main lapse, backlog-drain, and direct-claim paths.
Recommended Action
- Preserve equal-time predecessor rank across scheduler claims.
- Bound backlog suppression to a stable service deadline.
- Make the final suppression decision race-safe with review creation and add the corresponding regressions.
|
@ally please review exact head |
|
@ally please review exact head |
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: 44c888a
Prior Findings Dispositioned (3)
- prior:262b5b4 important 2 — fixed —
server/src/services/productivity-review.ts:1111— Fresh-claimed equal-time monitors are now retained in queue state independently of their claim-mutatedupdatedAt, so the production scheduler update no longer drops an in-flight predecessor from backlog grace. - prior:5bdb032 important 2 — still-present —
server/src/services/productivity-review.ts:1137— The static dispatch deadline is anchored to the due time, butlatestFreshPredecessorClaimedAtis recomputed on every pass; a failing predecessor reclaimed after each TTL movesfreshPredecessorDeadlineMsforward again and can suppress the source indefinitely. - prior:5bdb032 important 3 — still-present —
server/src/services/productivity-review.ts:1683— Revalidation now happens afterbeforeCreateOrUpdateReview, but it remains a separate read beforeissuesSvc.createat line 1707; a monitor claim or rearm committed in that interval still permits creation of the false review.
Important Issues (2)
- [prior:5bdb032 important 2]
server/src/services/productivity-review.ts:1137— Backlog suppression is still renewable rather than bounded to stable queue evidence. A predecessor whose dispatch repeatedly fails can be reclaimed every five minutes, and each newmonitorWakeRequestedAtadvances the derived deadline by another TTL. The negative control atserver/src/__tests__/productivity-review-service.test.ts:1101covers only an unclaimed, non-draining predecessor, not repeated production-shaped claims. Preserve an immutable service deadline for the source or cap claim-derived extension independently of later reclaims, then cover successive claims across multiple reconciliation passes. - [prior:5bdb032 important 3]
server/src/services/productivity-review.ts:1683— The final suppression check is not atomic with review creation. The new hook regression claims before this revalidation, but a scheduler claim or monitor rearm can still land after the check returns null and beforeissuesSvc.createwrites at line 1707. Serialize the source-state check with creation or make creation conditional on unchanged monitor state, and add a test hook in the revalidation-to-create window.
Strengths
- Equal-time claimed rows no longer disappear solely because the scheduler rewrites
updatedAt, and the scheduler now has a deterministicidtie-breaker. - The direct claim boundary matches scheduler eligibility at exactly the five-minute TTL.
- Open-review closure remains strict, avoiding a resolved-review snooze during a due-but-unserviced monitor gap.
Recommended Action
- Bound claim-derived backlog grace against immutable source/queue evidence.
- Make the final monitor suppression decision race-safe with review creation.
- Add production-shaped regressions for repeated predecessor reclaims and the revalidation-to-create interval.
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: 14b18d8
Prior Findings Dispositioned (2)
- prior:5bdb032 important 2 — fixed —
server/src/services/productivity-review.ts:1167— Fresh predecessor claims are now capped against a deadline derived from the original due time and queue position, so later reclaims cannot renew suppression indefinitely. - prior:5bdb032 important 3 — fixed —
server/src/services/productivity-review.ts:1760— The final monitor check now locks the source row inside the same issue-creation transaction and holds that lock through the review insert, serializing source claims and rearms against creation.
Important Issues (2)
- [gstack/native-codex]
server/src/services/issues.ts:8264— The new abortablebeforeInserthook runs afterallocateIdentifier()may create a Linear issue. Monitor suppression is an expected control-flow outcome that throws from this hook; PostgreSQL rolls back, but the external Linear issue then depends on best-effort deletion, whose failure is silently dropped. A normal monitor race can therefore leave a dangling external review issue. Run the guard before external identifier allocation while retaining its lock through insertion, or provide a pre-side-effect transactional hook for this use case, and add a Linear-provider suppression regression. - [gstack/native-codex]
server/src/services/productivity-review.ts:1193— The transactional final check locks only the source row; predecessor claim state is read without a coordinating lock. After this backlog query decides the absolute deadline has elapsed, a scheduler can commit a fresh claim for an earlier row before the review insert. That claim is durable evidence the queue is being serviced and can extend suppression within the bounded cap, but the stale query result still allows a false review. Coordinate scheduler claims and review creation with a shared lock or lock/revalidate the relevant queue state in a consistent order, and cover a predecessor claim in the backlog-query-to-insert window.
Strengths
- Backlog grace is now bounded against stable due-time evidence and has a repeated-reclaim negative control.
- Scheduler ordering is deterministic across
monitorNextCheckAt,updatedAt, andid, including equal-time claimed predecessors. - Source monitor claims and rearms are revalidated under a row lock in the insertion transaction, closing the previously reported source-row race.
Recommended Action
- Move the suppression guard ahead of external identifier side effects.
- Serialize fresh predecessor claims with the final backlog decision.
- Add focused regressions for both interleavings and run the embedded-Postgres suite.
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: f6379eb
Prior Findings Dispositioned (2)
- prior:14b18d8 important 1 — fixed —
server/src/services/issues.ts:8189— The abortable guard now runs beforeallocateIdentifier(), so a monitor suppression cannot create a Linear-side identifier before the transaction aborts. - prior:14b18d8 important 2 — fixed —
server/src/services/productivity-review.ts:1143— The final transaction now locks the source and qualifying queue rows, then recomputes backlog state while those locks are held through the review insert, preventing a predecessor claim from committing between revalidation and creation.
Important Issues (1)
- [gstack/native-codex]
server/src/services/productivity-review.ts:1143— The race fix takes an unboundedFOR UPDATElock over every qualifying predecessor across all active companies and holds those locks through the rest ofissuesSvc.create, including the documented Linear GraphQL call inallocateIdentifier(). A deep backlog plus a slow Linear request can therefore block the scheduler's first claim update and stall the entire sequential 50-monitor dispatch batch, as well as unrelated mutations to every locked issue. This directly prolongs the lapse-to-service gap the PR is intended to tolerate. Serialize the final decision with a short-lived advisory/queue lock or conditional insert rather than retaining a global predecessor row set across external side effects, and add a concurrency regression proving monitor claims are not blocked by review identifier allocation.
Strengths
- The suppression guard now precedes external identifier side effects.
- The bounded claim deadline prevents repeated predecessor reclaims from renewing grace forever.
- The focused tests cover direct and predecessor claims, TTL boundaries, equal-time ordering, negative controls, and final-window races.
Recommended Action
- Shorten or replace the queue-wide locking critical section before merge.
- Add a concurrency regression covering a slow identifier provider alongside monitor dispatch.
- Run the embedded-Postgres suite and required CI checks.
|
@ally review exact head |
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: ec1400f
Prior Findings Dispositioned (1)
- prior:f6379eb important 1 — still-present —
server/src/services/productivity-review.ts:1189— The predecessor row locks were replaced with a global advisory lock, but that lock is still held throughissuesSvc.create, including external identifier allocation. Scheduler claims use a non-blocking try-lock and therefore make no progress while creation is paused.
Important Issues (1)
- [prior:f6379eb important 1]
server/src/__tests__/productivity-review-service.test.ts:1675— The replacement lock still stalls monitor service globally during review creation. The new regression callstickDueIssueMonitorswhile the final create transaction is paused and explicitly expectstriggered === 0; line 1681 also confirms the predecessor remains unclaimed. Thus a slow or hung Linear identifier request no longer blocks the scheduler query, but it still causes every due monitor batch across all companies to be skipped until the review transaction releases the global lock, recreating the lapse-to-service gap this PR is meant to bound. Keep the queue lock only around a short atomic decision/claim boundary, or use a conditional insert/immutable queue epoch that does not suppress scheduler progress across external side effects; update the concurrency test to require the due predecessor to be claimed while review creation is paused.
Strengths
- The final source-state check runs before external identifier side effects and remains in the issue-creation transaction.
- The scheduler uses a non-blocking lock attempt, so its loop returns promptly rather than waiting on a slow review transaction.
- Backlog ordering, bounded claim grace, TTL boundaries, and long-lapsed negative controls have focused coverage.
Recommended Action
- Shorten the global queue critical section so monitor claims continue during identifier allocation.
- Change the concurrency regression to assert actual claim progress, not only a prompt zero-result return.
- Run the embedded-Postgres suite and required CI checks.
|
@ally review exact head |
|
@ally review exact head |
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: fa66701
Prior Findings Dispositioned (1)
- prior:f6379eb important 1 — fixed —
server/src/services/productivity-review.ts:1748— The advisory queue lock is now confined to the final revalidation transaction and is released beforeissuesSvc.create, so slow identifier allocation no longer makes scheduler claim attempts skip the entire due-monitor queue.
Important Issues (1)
- [gstack/native-codex/tests]
server/src/services/productivity-review.ts:1761— Releasing the queue and source-row locks beforeissuesSvc.createreopens the final check-to-insert race. A scheduler claim, monitor rearm, or source-state change can commit after the transaction at line 1758 returns but before creation starts here, and the stale decision still creates and wakes a productivity review. The concurrency regression pauses inbeforeFinalMonitorSuppressionRevalidation, so its claim lands before the protected read rather than in this post-commit window. Recommendation: establish a durable conditional review reservation/insert in the same short queue-locked transaction, then perform identifier allocation and other external side effects after that reservation; add a hook and regression between revalidation commit and create.
Strengths
- The previous global-lock stall is removed, and the scheduler now demonstrates real claim progress while review reconciliation is paused.
- Backlog ordering remains deterministic and claim-derived grace stays bounded against repeated predecessor reclaims.
- The focused suite covers direct claims, predecessor claims, TTL boundaries, long-lapsed negative controls, and Linear side-effect ordering.
Recommended Action
- Close the post-revalidation check-to-insert race before merge.
- Add a regression that claims or rearms the monitor after the lock transaction commits and before issue creation.
- Run the embedded-Postgres suite and required CI checks.
…ap (BLO-21003) A long_active_duration review fired on an episode whose own evidence block accounted 0m unattended, because deliberateFutureMonitor stopped suppressing the instant monitorNextCheckAt passed `now`, with no allowance for the dispatch pipeline (scheduler tick, K8s Job creation, pod scheduling) that follows before the wake is actually serviced. Widen the boundary by a bounded grace window, and stop flooring genuine sub-minute unattended residue to "0m" in the evidence line so a near-boundary case can't misreport as measured-zero. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Ally, please publish a fresh exact-head review for 2a6eaf2. The commitperclip review check has completed successfully on this head. The stale null-identifier/null-issueNumber reservation recovery finding from 6e859e6 is addressed with restart-safe finalization and regression coverage. Please state whether any Critical or Important issue remains at this exact SHA. |
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: 2a6eaf2
Prior Findings Dispositioned (1)
- prior:6e859e6 important 1 — still-present —
server/src/services/productivity-review.ts:754— Stale reservations are retried, but Linear identifier allocation still performs a fresh externalIssueCreatewithout durable recovery state. If Linear creates the issue and the worker dies before lines 764-788 commit its identifier/link, the local transaction rolls back and the next stale-recovery pass creates a second Linear issue; the in-process compensation at line 794 cannot run after process loss.
Important Issues (3)
- [prior:6e859e6 important 1]
server/src/services/productivity-review.ts:754— Reservation recovery is not idempotent for Linear-backed companies.allocateIdentifier()calls LinearIssueCreatewithout a stable idempotency key (server/src/services/identifier-allocator.ts:322), while the returned external ID is only persisted later in the same local transaction. A crash or ambiguous HTTP response after Linear commits but before the local commit leaks the first Linear issue and recovery creates another. Persist a durable external-allocation intent/idempotency key before the call, or reconcile the reservation to the already-created Linear issue; add a process-loss/ambiguous-response regression for the Linear provider. - [code/gstack/native-codex]
server/src/services/productivity-review.ts:1828— Concurrent stale-recovery passes both emit creation side effects. They can both read the same stale reservation; the finalization advisory lock serializes them, but the second call returns the already-finalized row at line 749 and both callers then executefinishCreatedProductivityReview()at line 1834. This duplicatesissue.productivity_review_createdactivity and assignment wakeups. Return whether this call actually finalized the reservation, or make the activity/wakeup path durably idempotent; add a two-reconciler stale-recovery test. - [gstack/native-codex]
server/src/services/productivity-review.ts:1834— Recovery commits identifier finalization before recording creation activity and enqueueing the assignment wake. If the worker dies after finalization but beforefinishCreatedProductivityReview(), later passes see a normal identified review at lines 1846-1854 and never retry the missing side effects; a failure after line 993 can similarly leave activity without a wake. Persist a finalization/outbox state and replay it idempotently so both activity and wake delivery survive process loss.
Strengths
- The stale internal-provider reservation now receives an identifier instead of remaining permanently unactionable.
- The reservation age guard avoids interfering with an ordinary in-flight finalization.
- The focused recovery test verifies the basic internal identifier, activity, and wakeup path.
Recommended Action
- Make external Linear allocation recoverable without duplicate side issues.
- Make stale recovery and its activity/wakeup side effects idempotent under concurrency and process loss.
- Add Linear-provider, concurrent-recovery, and post-finalization restart regressions before merge.
|
Ally, please re-review exact head 00cece6. Follow-up for the three Important findings on 2a6eaf2:
Regression coverage added for Linear existing-id reuse, Linear create idempotency key submission, ambiguous Linear create recovery, concurrent stale reservation recovery, and post-finalization side-effect replay. Local verification on this head:
Please state whether any Critical or Important issue remains at this exact SHA. |
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: 00cece6
Prior Findings Dispositioned (3)
- prior:6e859e6 important 1 — still-present —
server/src/services/productivity-review.ts:805— Linear creation now uses the reservation UUID, but an ambiguous create is recovered with only one immediate lookup; if that lookup errors or returns null after Linear committed, this catch deletes the local reservation at lines 809-812, so the next reconciliation allocates a new UUID and can create a duplicate Linear issue. - prior:2a6eaf2 important 2 — fixed —
server/src/services/productivity-review.ts:758— Finalization now returnsfinalized: falseto the losing caller under the per-review advisory lock, and the separately locked side-effect replay lets only the caller that changed durable state report creation. - prior:2a6eaf2 important 3 — still-present —
server/src/services/productivity-review.ts:1108— The replay path awaitsenqueueWakeup()but ignores its nullable result, setswakeAlreadyProcessed = trueunconditionally at line 1129, and writes the durable “wake enqueued” marker even when scheduling returnednullwithout a queued/coalesced run.
Important Issues (2)
- [prior:6e859e6 important 1]
server/src/services/productivity-review.ts:805— Linear-backed recovery still fails closed by deleting the idempotency anchor after an unconfirmed create. A timeout followed by a transient/replica-laggedIssueByIdmiss reaches this catch withcreatedLinearIssueId === null; deleting the null-identifier reservation frees the active-review slot, and the next pass reserves a different UUID even though the first Linear issue may exist. Preserve the reservation on ambiguous external allocation failures and retry lookup/create with the same UUID until the outcome is known; add a regression where the post-create lookup initially fails or returns null and a later reconciliation finds the original Linear issue. - [prior:2a6eaf2 important 3]
server/src/services/productivity-review.ts:1129— A wake suppression is persisted as successful delivery. ProductionheartbeatService.enqueueWakeup()returnsnullfor temporary scheduling suppression and other non-queued outcomes, but this path marks it processed and insertsissue.productivity_review_assignment_wake_enqueued; future reconciliations then skip the wake permanently. Only write the marker when the returned run/request proves a durable queued or coalesced wake exists, or confirm the idempotency key inagent_wakeup_requestsafter the call; add a regression where the first enqueue returnsnulland the next reconciliation retries successfully.
Strengths
- The stable Linear issue ID supports lookup-before-create and recovery after a confirmed ambiguous response.
- Per-review advisory locks and the
finalizedresult prevent concurrent stale-recovery callers from duplicating the normal success path. - Side-effect replay has stable activity names and a deterministic wake idempotency key, providing the right durable primitives once unsuccessful enqueue outcomes are distinguished.
Recommended Action
- Retain the reservation across unconfirmed Linear allocation outcomes.
- Mark assignment wake completion only after durable enqueue evidence exists.
- Add the two failure/retry regressions and run the embedded-Postgres suites in CI.
|
Ally, please re-review exact head 004aa34fcd373734b34314d1dc43fd5949518a57. Follow-up for the two remaining Important findings on 00cece6:
Local verification on this head:
Please state whether any Critical or Important issue remains at this exact SHA. |
|
@ally please publish a fresh exact-head review for 004aa34. The commitperclip review check is green on this head. The two remaining findings from 00cece6 were addressed: unconfirmed Linear creates now preserve the reservation/idempotency key, and null enqueue results no longer write the wake-enqueued marker unless durable queued/coalesced request evidence exists. Please state whether any Critical or Important issue remains at this exact SHA. |
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: 004aa34
Prior Findings Dispositioned (2)
- prior:6e859e6 important 1 — still-present —
server/src/services/identifier-allocator.ts:173— Retries preserve the reservation UUID after an unconfirmed create, but the retry's initialIssueByIdlookup is outside the ambiguity-conversiontry; a transient lookup failure remains generic and causesfinalizeReservedProductivityReviewIssueto delete the stable reservation atserver/src/services/productivity-review.ts:822. - prior:2a6eaf2 important 3 — fixed —
server/src/services/productivity-review.ts:1143— A null enqueue result now requires a durable queued/claimed/coalesced/deferred request with the same idempotency key before the wake marker is written.
Important Issues (3)
- [prior:6e859e6 important 1]
server/src/services/identifier-allocator.ts:173— The Linear idempotency anchor can still be lost during recovery. If an earlierIssueCreatecommitted ambiguously and the next reconciliation's preliminaryIssueByIdcall fails transiently, that error bypassesLinearIssueCreateUnconfirmedError; the finalizer then deletes the null-identifier reservation and a later pass allocates a new UUID, permitting a duplicate Linear issue. Convert failures of this preliminary lookup into the same unconfirmed outcome and retain the reservation; add a regression where the retry lookup errors before a later pass finds the original issue. - [gstack/native-codex]
server/src/services/productivity-review.ts:1937— Reservation recovery still depends on the source re-entering the normal unsuppressed candidate path. If the worker dies after committing a reservation and the source becomes terminal, gains a pending monitor, or otherwise stops producing evidence, this branch is never reached; the active null-identifiertodoreview remains unactionable and blocks replacement indefinitely. Recover stale reservations in an independent sweep or before source candidate/suppression filtering, with a regression where the source changes state after reservation. - [gstack/native-codex]
server/src/services/productivity-review.ts:75—completedwake requests are excluded from durable delivery evidence. If enqueue commits, the wake finishes, and the finishing transaction/process then fails before writing the activity marker, the next reconciliation treats the completed request as absent and enqueues the same assignment again despite the stable idempotency key. Count terminal successful delivery as processed, or make enqueue idempotency replay completed requests; add a process-loss regression between enqueue completion and marker commit.
Strengths
- The reported null-enqueue path now avoids writing a false success marker and retries on the next reconciliation.
- Linear create retries use the reservation UUID as
IssueCreateInput.id, and confirmed delayed visibility reuses the original external issue. - Queue locking and review reservation establish a short linearization point without holding the monitor queue across external identifier allocation.
Recommended Action
- Preserve the Linear reservation across every unconfirmed lookup/create outcome.
- Make stale reservation recovery independent of source candidacy.
- Treat completed idempotent wakes as durable prior delivery and cover the process-loss window.
|
Supplementary pass: a concurrent Ally run already submitted the operative verdict for this exact head. This non-duplicate finding is posted as a comment, not a second review. Ally — Additional FindingReviewed head: 004aa34 Critical Issues (1)
|
|
Ally, please re-review exact head a37ff015db1d015946544287f175e9731bc330a7.\n\nChanges since 004aa34:\n- Linear stable-id preliminary lookup failures now throw LinearIssueCreateUnconfirmedError, preserving the reservation instead of deleting the idempotency anchor\n- stale reserved productivity-review rows are recovered in a pre-candidate sweep, so terminal/suppressed sources do not leave null-identifier reviews stuck\n- completed assignment wake requests count as durable wake delivery evidence before writing the wake marker\n- added regressions for retry lookup errors, stale reservation after source leaves candidates, and completed wake process-loss replay\n\nLocal checks:\n- pnpm --filter @paperclipai/server typecheck\n- pnpm exec vitest run server/src/tests/allocate-identifier-linear-happy.test.ts server/src/tests/productivity-review-service.test.ts --no-file-parallelism --maxWorkers=1 (embedded Postgres suites skipped on this host)\n- git diff --check |
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: a37ff01
Prior Findings Dispositioned (3)
- prior:6e859e6 important 1 — fixed —
server/src/services/identifier-allocator.ts:174— The preliminary stable-ID lookup is now wrapped, and lookup failures are converted toLinearIssueCreateUnconfirmedError, preserving the reservation for a later retry with the same UUID. - prior:004aa34 important 2 — fixed —
server/src/services/productivity-review.ts:2246— Stale null-identifier reservations are now recovered before source candidate filtering, so a source leaving the active candidate set no longer strands the reservation solely because normal evidence collection does not revisit it. - prior:004aa34 important 3 — fixed —
server/src/services/productivity-review.ts:75— Completed wake requests now count as durable delivery evidence, preventing replay from enqueueing a second assignment wake after completion won the race with marker persistence.
Critical Issues (1)
- [code/gstack/native-codex]
server/src/services/productivity-review.ts:1111— The production assignment-wake path still self-deadlocks.finishCreatedProductivityReview()updates the review row inside its transaction, retains that row lock, and then awaitsdeps.enqueueWakeup()at line 1133. The real heartbeat enqueue opens another transaction and executesSELECT ... FOR UPDATEon the same review atserver/src/services/heartbeat.ts:23883, so the inner transaction waits for the outer lock while the outer transaction waits for enqueue to return. Commit the issue/activity transaction before enqueueing with the stable idempotency key, then persist the wake marker in a separate short transaction (or use an outbox); add an integration regression using the real heartbeat service rather than a mock enqueue function.
Important Issues (2)
- [code/tests/gstack]
server/src/services/productivity-review.ts:2313— Stale-reservation recovery finalizes and wakes every reservation without checking whether its source is now terminal or otherwise no longer reviewable. Because the close sweep runs before recovery, the newdone-source test intentionally creates and wakes an obsolete review that the next reconciliation will close. Revalidate source reviewability before allocation; atomically retire terminal-source reservations without assigning an identifier or waking a manager, and change the regression to assert no wake. - [gstack/native-codex]
server/src/services/productivity-review.ts:826— A confirmed idempotent Linear recovery can still lose its anchor on a later local failure. WhenIssueByIdfinds the existing Linear issue,createdLinearSideIssueis false, so a subsequent issue update, link insert, or commit failure reaches this generic catch withcreatedLinearIssueId === nulland deletes the reservation at line 833. The external issue remains, but the next reconciliation allocates a new reservation UUID and can create a duplicate Linear issue. Preserve the reservation after any successful Linear lookup/allocation and retry local finalization with the same UUID; only delete when failure is proven to precede external allocation.
Strengths
- Stable-ID lookup failures now retain the Linear idempotency anchor instead of falling through to a new reservation.
- Reservation discovery is independent of active-source candidate filtering, and completed wake requests are recognized as durable prior delivery.
- The monitor queue ordering, bounded grace, and short reservation linearization point have extensive focused regression coverage.
Recommended Action
- Remove the nested row-lock self-deadlock before merge.
- Retire obsolete terminal-source reservations without manager wakes.
- Preserve the Linear reservation across post-lookup local failures and run the embedded-Postgres integration targets.
|
Ally, please re-review exact head 2e832fcf0b2d6662b067d819994b8083940dd712.\n\nAdditional follow-up since a37ff01 for the supplementary self-deadlock finding on 004aa34:\n- productivity review finish now claims/repairs created/wake state in short transactions only\n- assignment wake enqueue runs after the finish transaction commits, so production enqueue can take its own review row lock without deadlocking\n- a fresh wake-started activity fences concurrent finishers; null/throwing enqueue releases the claim with wake_failed so immediate retry still works\n- added a row-lock replay regression where enqueueWakeup opens a transaction and SELECTs the review FOR UPDATE\n\nIncludes the prior a37ff01 fixes for Linear unconfirmed lookup, stale reservation pre-candidate recovery, and completed wake durable evidence.\n\nLocal checks:\n- pnpm --filter @paperclipai/server typecheck\n- pnpm exec vitest run server/src/tests/allocate-identifier-linear-happy.test.ts server/src/tests/productivity-review-service.test.ts --no-file-parallelism --maxWorkers=1 (embedded Postgres suites skipped on this host)\n- git diff --check |
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: 2e832fc
Prior Findings Dispositioned (3)
- prior:a37ff01 critical 1 — fixed —
server/src/services/productivity-review.ts:1377— The wake claim transaction now commits beforeenqueueWakeup()is called at line 1388, so the real heartbeat service can lock the review row without waiting on its caller's retained row lock. - prior:a37ff01 important 1 — still-present —
server/src/services/productivity-review.ts:2550— Stale-reservation recovery still finalizes every reservation with an existing source and then wakes its assignee; it does not reject terminal or otherwise no-longer-reviewable sources before allocation. The current regression atserver/src/__tests__/productivity-review-service.test.ts:2143explicitly expects a review and wake after the source is alreadydone. - prior:a37ff01 important 2 — still-present —
server/src/services/productivity-review.ts:836— A successful idempotent Linear lookup leavescreatedLinearIssueIdnull, so any later local update/link/commit failure reaches this catch, deletes the stable null-identifier reservation at lines 839-842, and allows a later pass to allocate a new UUID for the already-existing external issue.
Important Issues (3)
- [prior:a37ff01 important 1]
server/src/services/productivity-review.ts:2550— Recovery creates and assigns an obsolete productivity review when its source became terminal after reservation. Revalidate source reviewability before identifier allocation and atomically retire terminal-source reservations without emitting creation activity or an assignment wake; change the terminal-source regression to require no wake. - [prior:a37ff01 important 2]
server/src/services/productivity-review.ts:836— Confirmed Linear recovery can still lose its idempotency anchor after a local finalization failure. Preserve the reservation after any successful Linear lookup/allocation and retry the local update/link with the same UUID; delete only when failure is proven to precede external allocation. - [code/tests/gstack/native-codex]
server/src/services/productivity-review.ts:1355— Wake-claim lease time mixes two clocks. Fresh claims are stamped with the reconciliation-wideevidence.generatedAtat line 1374 but are considered active againstDate.now(). A scan or prior work lasting five minutes therefore inserts an already-expired claim, allowing another reconciler to callenqueueWakeupconcurrently. Retries that reuse the same generated time also leavestartedandfailedrows tied, while the ordering at lines 1103-1111 always choosesfailedahead of the newer start and prevents successful completion from recording its marker. Stamp and compare claims using one transaction/database clock, preserve event order independently of evidence time, and add long-scan plus same-time failed-then-successful retry regressions.
Strengths
- Moving the real enqueue outside the review-locking transaction resolves the previously reported self-deadlock.
- The new advisory claim separates durable coordination from the external wake side effect and includes explicit failure/retry markers.
- Existing coverage continues to exercise bounded monitor grace, queue ordering, reservation recovery, and assignment-wake replay.
Recommended Action
- Retire obsolete terminal-source reservations without finalization or wake delivery.
- Preserve the Linear reservation through post-lookup local failures.
- Use one authoritative clock and deterministic sequence for wake-claim leases and retries.
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: b4dd226
Prior Findings Dispositioned (3)
- prior:a37ff01 important 1 — still-present —
server/src/services/productivity-review.ts:2633— Recovery now retires terminal sources, but it does not apply the normal reviewability conditions before finalization; hidden, blocked, in-review, user-assigned, or reassigned sources still reach finalization at line 2649, while a disappeared source is left as a permanent active reservation at line 2630. - prior:a37ff01 important 2 — fixed —
server/src/services/productivity-review.ts:810— Any confirmed Linear allocation now setspreserveReservationbefore the local issue/link writes, so a later local failure retains the stable UUID anchor for retry. - prior:2e832fc important 3 — fixed —
server/src/services/productivity-review.ts:1387— Wake leases are now stamped and compared with the same database clock, and line 1404 adds a monotonic attempt sequence that disambiguates equal-time failed/retried claims.
Important Issues (1)
- [prior:a37ff01 important 1]
server/src/services/productivity-review.ts:2633— Stale reservation recovery still does not revalidate whether the source remains reviewable. Only terminal status is handled; a source that became hidden, blocked,in_review, user-assigned, unassigned, or reassigned to another agent is finalized and wakes the reviewer selected from stale source ownership. If the source was deleted, line 2630 leaves the null-identifier reservation active forever. Apply the same status, visibility, and assignment checks asevaluateSourceReviewability()before allocation, then atomically retire every no-longer-reviewable or missing-source reservation without a creation activity or assignment wake. Add regressions for reassignment/non-candidate status and a missing source.
Strengths
- Confirmed Linear lookup/allocation now preserves the reservation UUID through downstream local failures.
- Assignment-wake claims use one authoritative clock and deterministic attempt sequencing, closing the long-scan and equal-time retry races.
- Terminal-source recovery now retires the reservation without allocating an identifier or waking a manager.
Recommended Action
- Revalidate all source reviewability conditions during stale reservation recovery.
- Retire missing and otherwise no-longer-reviewable reservations without side effects.
- Add focused recovery regressions and run the embedded-Postgres suite.
Status check on the latest Ally finding + current CIAlly's Important Issue from the
Retirement never allocates an identifier or fires a creation/assignment-wake side effect (verified by 3 new regressions: non-candidate status ( However, CI on this exact head is red, and I don't think it should be re-reviewed or merged until that's resolved:
None of these are new — I traced each to the commit that introduced it:
None of them are from the latest commit ( Given the PR checklist item "All Paperclip CI gates are green" is unchecked and CI is genuinely red, requesting another Ally pass right now would be premature. Recommend: let CI finish on a head before pushing the next fix (or squash the pending fixes and push once), then re-request review once I'll keep tracking this from BLO-21003. |
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: fe6e6ba
Prior Findings Dispositioned (1)
- prior:a37ff01 important 1 — still-present —
server/src/services/productivity-review.ts:2698— Recovery now checks terminal, hidden/non-candidate, reassigned, and missing sources, but those reads are not atomic with finalization. A source can become done, hidden, reassigned, or opted out after validation and beforefinalizeReservedProductivityReviewIssue(), which does not re-read or lock the source.
Important Issues (3)
- [prior:a37ff01 important 1]
server/src/services/productivity-review.ts:2698— Stale-reservation recovery has a source-state TOCTOU gap. After all reviewability checks complete, another transaction can make the source unreviewable before line 2699 finalizes the reservation; the finalizer predicates only on the review's null identifier fields, so it can still allocate a Linear issue, record creation, and wake the stale owner. Revalidate the source and finalize or retire the reservation under one coordinating lock/transaction, and make finalization reject a reservation already retired by a concurrent reconciler. Add a regression that transitions the source in this exact pre-finalization window. - [code/gstack/native-codex]
server/src/services/productivity-review.ts:2676— Failure to resolve a current review owner authorizes the old owner. If the reserved manager becomes paused, terminated, pending approval, or budget-blocked and no fallback is eligible,resolveReviewOwnerAgentId()returnsnull; the guarded mismatch condition is skipped and recovery finalizes and wakesreview.assigneeAgentIdanyway. Treat a null current owner as non-finalizable (retire or defer), matching the normal creation path, and cover an unavailable/budget-blocked owner. - [errors/gstack/native-codex]
server/src/services/productivity-review.ts:2538— Retirement can orphan an ambiguously-created Linear side issue.LinearIssueCreateUnconfirmedErrordeliberately preserves the null-identifier reservation as the stable external idempotency anchor, but if the source becomes unreviewable before lookup confirms the create, this path marks that reservation done without reconciling or deleting the possible Linear issue. Done reservations are excluded from later recovery, so the external issue can remain permanently unlinked. Resolve the stable Linear ID before retirement or retain a durable cleanup state, and add an ambiguous-create-then-source-transition regression.
Strengths
- New reservations persist the original source-agent identity, and legacy reservations fail closed when source ownership cannot be reconstructed.
- The added tests cover terminal, non-candidate, owner-change, same-owner reassignment, and missing-source retirement without creation or wake side effects.
- Linear allocation retains a stable UUID across ambiguous outcomes, and wake replay uses durable idempotency evidence.
Recommended Action
- Make source revalidation and reservation finalization/retirement atomic.
- Do not finalize when no eligible current review owner exists.
- Preserve or clean up the Linear idempotency anchor when retiring an ambiguous reservation.
allyblockcast
left a comment
allyblockcast
left a comment
There was a problem hiding this comment.
Approved with the allyblockcast reviewer seat after green automated review gates.
3 participants
Thinking Path
Linked Issues or Issue Description
Paperclip issue: BLO-21003
Related incident: BLO-19772
Supersedes app-authored PR: #980
What happened?
deliberateFutureMonitorstopped suppressinglong_active_durationreviews the instantmonitorNextCheckAtpassednow, even when the wake had not yet had time to be serviced. A fixed grace still missed large due-monitor backlogs becausetickDueIssueMonitorsonly claims 50 monitors per pass.Expected behavior
A monitor that lapsed only by normal scheduler, dispatch-claim, or bounded backlog latency should still suppress creation of a stale-review escalation. An already-open review should not be auto-closed during that due-but-unserviced gap because closing it creates a resolved-review snooze before dispatch succeeds.
Steps to reproduce
monitorNextCheckAtjust beforenow.long_active_durationreview for a monitored issue.Paperclip version or commit
PR head
521f486be9c86710ef5c84065e02232860b40194against Paperclipmaster.Deployment mode
Self-hosted Paperclip server / productivity review service.
What Changed
monitorNextCheckAtis still in the future.monitorWakeRequestedAtclaim as durable evidence that a lapsed monitor wake is still being serviced.long_active_durationreview.Verification
git diff --checkpassed.pnpm --filter @paperclipai/server typecheckpassed.pnpm exec vitest run server/src/__tests__/productivity-review-service.test.tsloaded successfully but skipped locally because embedded Postgres support is unavailable in this host environment; CI should execute the embedded suite on the runner.Risks
Low to moderate. The new-review suppression window intentionally delays review creation while the monitor scheduler can still plausibly service the due wake. The close path is conservative and no longer resolves already-open reviews during a due-but-unserviced gap.
Model Used
OpenAI Codex GPT-5 implemented the final review fixes, tests, typecheck, and independent-author handoff.
Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template