fix(sessions): harden the pipeline watch against races, outages and lost owners - #1534
Merged
srtab merged 1 commit intoAug 26, 2026
Merged
Conversation
…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
force-pushed
the
claude/objective-mcclintock-2b969a
branch
from
August 26, 2026 15:04
3a9e838 to
5e1b618
Compare
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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, bothget_latest_pipeline_for_refoptimizations 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
watch_statewrite._atransitionis 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 awhere=.canceled.get_pipelinestops answeringNoneon 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.PositiveSmallIntegerFieldis a sibling ofPositiveIntegerField, not a subclass, so the roster of three silently returned the raw string. Matching onmodels.IntegerFieldcovers every width.The watch's owner
aarm_watchalready took the publishing run'suser_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
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 splitis_transient_bus_errorand_is_transient_mcp_erroralready draw. Auth counts as transient: DAIV's project-scoped tokens are ephemeral and expire mid-watch by design.WATCH_SWEEP_LIMITas the re-judge sweep.Notifications
The give-up emit moves out of
sessions/pipeline_watch.pyintonotifications/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_relatedon the owner the fix-run dispatch reads.Testing
4773 passed— the fullmake testsuite, 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.