Skip to content

fix(server): discard queued updates when pull request watches end - #30894

Draft
OleKristianH wants to merge 1 commit into
pingdotgg:mainfrom
OleKristianH:fix/drop-stale-pr-wakeups
Draft

OleKristianH wants to merge 1 commit into
pingdotgg:mainfrom
OleKristianH:fix/drop-stale-pr-wakeups

Conversation

@OleKristianH

Copy link
Copy Markdown

Problem

PR-watch updates already queued behind an active turn survive unwatch, merge, and unlink. In an installed Linux desktop nightly (0.1.8-nightly.20261104.4165, source 8b44f42b), 19 such updates replayed after the watched PR had merged and the watch had ended, repeatedly waking completed work.

Reproduce by keeping a turn active, letting several watched-PR updates queue, ending the watch before the turn finishes, then allowing the queue to drain. The old updates should no longer start provider turns.

Change

Persist the PR key and watch start time on generated monitor notifications. When a PR-link command ends a watch, cancel only its queued runs through the existing run/attempt/node cancellation path. Preserve active work, user messages, other monitor notifications, and other watched PRs. For legacy notifications without a stamp, discard only server-created message:pr-watch:* entries after every watch in that thread has ended. The cancellation events survive projection rebuild.

The optional shared-contract field decodes existing notifications; this changes shared server behavior for every client and provider, without a UI change. The user guide explains the cancellation behavior.

Scope and approval

Refs #21009, confirmed by a maintainer.

Related implementation: #21239 is already open. This is a narrower alternative that cancels at watch teardown and also retires existing untagged queued updates. It does not implement newer-head supersession or queue coalescing, and should not close the whole issue. This PR is submitted for comparison rather than as a request to land both overlapping approaches.

Verification

  • Added three real-orchestrator/SQLite regressions for unwatch, merge, and unlink. All three failed before the production change because obsolete runs remained queued, then passed with the fix.
  • Each scenario includes two watched PRs, repeated updates, active work, a user follow-up, another monitor, legacy queued updates, and projection replay. Ending one watch cancels only its updates; ending the last also cancels legacy updates.
  • Focused command: pnpm exec vp test run apps/server/src/orchestration-v2/runtimeLayer.test.ts -t 'discards queued PR updates when the watch ends' (3 passed).
  • V8 coverage measured 20/20 changed executable statement lines covered. This is changed-line coverage, not full-file coverage.
  • After integration with upstream main 9e855385, pnpm exec vp run check:changed --concurrency 1 --timeout 900 passed format, lint, the first unused-code scan, GraphQL generation checks, typechecks for all 36 affected packages, and all 147 tests across the five selected files (including the full runtime test file).
  • The second unused-code scan timed out after 900 seconds inside check:changed (overall exit 1). The subsequent mandatory commit hooks completed successfully, including the full vp run knip:check, format, and lint. No check or hook was bypassed.

Runtime observation (UTC, read-only live-state inspection):

10:03:53–12:15:09  19 PR-watch runs queued behind active work
12:17:10           PR merged
12:21:40–12:27:47  queued notices replayed as new turns after completion

Limitations: check:changed was not rerun after the full unused-code scan passed in the commit hook. CI still owns the repository-wide suite, build, and release smoke. No live installation/database was modified, no real provider turn was launched, and no UI/platform-specific pass was performed.

Implemented with GPT-6.1-Sol through the Codex harness in T3 Code.

@genesiscz

genesiscz commented Nov 5, 2026 •

Copy link
Copy Markdown

A real-world case that matches this teardown path, on 0.1.7 with the Claude provider: one busy thread watched about 25 PRs, the agent unwatched all of them between 12:58 and 15:58 UTC, and 115 unstamped PR-watch notices stayed queued afterwards. The agent cancelled them by hand, one t3_queue call each. Details and the source check against current main: #21009 (comment), #26987 (comment).

With this PR, the legacy rule would have cleared all 115 at the last unwatch. Notices stamped after the upgrade would go at each PR's own unwatch, which is the better case: in this thread the unwatches were spread over three hours.

Notices that are accepted while the watch is ending, and a newer head for a watch that stays on, still need the delivery check from #21239, as the description says. Per-PR coalescing (10 to 13 queued notices per PR here) is #26987.

Written by Claude Opus 5.5 in T3 Code for the reporter.

@aravhawk aravhawk 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.

Read the teardown cancellation against the #21009 report and genesiscz's 115-notice case. One question on coverage outside the PR-link command path.

};
}
})();
if (command.type.startsWith("thread.pull-request")) {

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 gate only runs for thread.pull-request* commands, which matches the unwatch / merge-via-link.sync / unlink cases in the new tests. The next test in this file is "settling ends every pull request watch" — if settlement (or thread.stop) clears watches without going through a pull-request command, would queued message:pr-watch:* wakes still replay the way they do on main today?

If settlement already ends watches by dispatching thread.pull-request.watch with watching: false, this is fine and the docs are complete. If it mutates watch state some other way, the 115-notice case genesiscz described on a busy thread could still happen when the thread settles rather than when each PR is unwatched. Happy to treat that as follow-up either way; just wanted to check the boundary before this lands next to #21239.

This branch has not been deployed

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

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants