Skip to content

fix(sessions): harden the pipeline watch against races, outages and lost owners - #1534

Merged
srtab merged 1 commit into
claude/cool-carson-edd8dbfrom
claude/objective-mcclintock-2b969a
Aug 26, 2026
Merged

fix(sessions): harden the pipeline watch against races, outages and lost owners#1534
srtab merged 1 commit into
claude/cool-carson-edd8dbfrom
claude/objective-mcclintock-2b969a

Conversation

@srtab

@srtab srtab commented Aug 26, 2026

Copy link
Copy Markdown
Owner

Stacked on #1529 — review that one first; this PR's diff is only the commit on top. Rebased onto feat(sessions): arm the CI watch from every publishing seam, which independently landed the partial sweep index, both get_latest_pipeline_for_ref optimizations and the webhook gate reordering — those are dropped here rather than duplicated.

A hardening pass over the pipeline watch, covering the failure modes the preceding commits left open.

Correctness

  • One compare-and-swap for every watch_state write. _atransition is now the only writer — the evaluation's four outcomes, the no-diff exhaust and the reconciler's own transitions. Two events finishing together both pass their checks off their own stale read, and only the row count says which one owns the transition; without it each posts its own MR comment, which no constraint dedupes. It subsumes _aclose_watch, and carries the dispatch claim's cap re-assertion as a where=.
  • Webhook pipelines are correlated against the MR head too, not just polled ones. A webhook names which pipeline it reports, not whether that pipeline is still the head: GitLab auto-cancels redundant pipelines, so the push after ours makes the older one emit a terminal canceled.
  • get_pipeline stops answering None on 401/403/5xx. Both clients collapsed every failed GET to "no pipeline", so an outage read as no pipeline yet — which the watch waits out in silence. Only a 404 is absence now; anything else propagates, leaves the watch armed, and gets retried by the next event or sweep.
  • Site settings coerce every int width. PositiveSmallIntegerField is a sibling of PositiveIntegerField, not a subclass, so the roster of three silently returned the raw string. Matching on models.IntegerField covers every width.

The watch's owner

aarm_watch already took the publishing run's user_id; this adds the other half — an existing owner is never reassigned, because the MR thread may be a human's merge-request conversation rather than a session DAIV created.

Observability

  • Transient platform failures (is_transient_platform_error) log at WARNING without a traceback, so an hours-long GitLab/GitHub outage does not mint one Sentry error per sweep — the same split is_transient_bus_error and _is_transient_mcp_error already draw. Auth counts as transient: DAIV's project-scoped tokens are ephemeral and expire mid-watch by design.
  • The reconciler's 6-hour give-up now logs a warning and posts a note on the merge request. It is where every unresolved failure lands — unreadable pipeline, dead fix run, pipeline that never started — and closing it silently made those indistinguishable. Bounded by the same WATCH_SWEEP_LIMIT as the re-judge sweep.

Notifications

The give-up emit moves out of sessions/pipeline_watch.py into notifications/watch_notifiers.py: reading the pipeline is the watch's job; the source key, channels, dedup and payload are the notifications app's. It is deduped per (thread_id, pipeline_id) rather than per thread — a thread outlives the merge request and a watch can be re-armed with a fresh budget, so a thread-only key would mute every later give-up. Adds the RocketChat renderer the event was missing.

Cost

One platform client per evaluation — constructing a GitHub one costs a live installation lookup, and the read, the correlation and the note were paying for three. Plus select_related on the owner the fix-run dispatch reads.

Testing

4773 passed — the full make test suite, including new coverage for the transition race, the head-sha correlation, the non-404 propagation, the never-reassigned owner, the reconciler's give-up note and the give-up notifier's dedup constraint.

…ost owners

Follow-up pass over the pipeline watch, covering the failure modes the
preceding commits left open:

- Every watch_state write goes through one compare-and-swap helper
  (_atransition), including the reconciler's own transitions and the
  no-diff exhaust. Two events finishing together both pass their checks
  off their own stale read, and only the row count says which one owns
  the transition — without it each posts its own MR comment, which no
  constraint dedupes.
- Correlate webhook pipelines against the MR head too, not just polled
  ones: a webhook names which pipeline it reports, not whether that
  pipeline is still the head, and GitLab auto-cancels redundant
  pipelines so the push after ours makes the older one emit a terminal
  canceled.
- get_pipeline no longer answers None on 401/403/5xx on either platform,
  which made an outage read as "no pipeline yet" — something the watch
  waits out in silence. Only a 404 is absence; anything else propagates,
  leaves the watch armed, and is retried by the next event or sweep.
- Platform failures log at WARNING without a traceback when transient
  (is_transient_platform_error), so an hours-long outage does not mint
  one Sentry error per sweep — the same split is_transient_bus_error and
  _is_transient_mcp_error draw.
- aarm_watch attributes the MR thread it creates to the originating run's
  owner without ever reassigning an existing one: the thread may be a
  human's MR conversation.
- The give-up notification moves to notifications/watch_notifiers.py,
  deduped per (thread, pipeline) rather than per thread — a thread
  outlives the merge request and a watch can be re-armed with a fresh
  budget. Adds the RocketChat renderer the event was missing.
- The reconciler's 6-hour give-up posts a note and logs a warning instead
  of closing the watch silently: it is where every unresolved failure
  lands, and closing it silently made those indistinguishable.
- Site settings coerce every IntegerField width, not a roster of three,
  so PositiveSmallIntegerField no longer returns the raw string.
- One platform client per evaluation, and select_related on the owner the
  fix-run dispatch reads.
@srtab
srtab force-pushed the claude/objective-mcclintock-2b969a branch from 3a9e838 to 5e1b618 Compare August 26, 2026 15:04
@srtab srtab closed this Aug 26, 2026
@srtab srtab reopened this Aug 26, 2026
@srtab srtab closed this Aug 26, 2026
@srtab srtab reopened this Aug 26, 2026
@srtab
srtab merged commit bee1452 into claude/cool-carson-edd8db Aug 26, 2026
8 of 13 checks passed
@srtab
srtab deleted the claude/objective-mcclintock-2b969a branch August 26, 2026 21:06
srtab added a commit that referenced this pull request Aug 27, 2026
The remote branch had moved on while main was being merged in: PR #1534
("harden the pipeline watch against races, outages and lost owners"). Two
textual conflicts, both sides kept:

- notifications/run_notifiers.py — the two sides changed adjacent lines for
  unrelated reasons. Keep the remote's `enabled_channel_types()` helper *and*
  main's `BatchRow.from_values(...)` over `values(*BatchRow.COLUMNS)`, which
  adds the `envelope__summary` column; `from_values` is keyed by column name
  because the positional splat it replaces would silently swap `status` and
  `summary`.
- tests/.../test_rocketchat_renderers.py — two independent test classes
  appended at the same point.

One semantic break the textual merge hid:

- tests/.../test_watch_state.py — the remote side's new index-literals test
  imports the pipeline-watch migration by module name, which the previous
  commit renumbered to 0008 to clear main's own 0007.

5001 unit tests pass, ruff clean, `makemigrations --check` detects no drift
(sessions at 0008, notifications at 0009), mkdocs --strict builds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant