Skip to content

Clear AI messages abandoned in processing - #5071

Open
elias-ba wants to merge 1 commit into
timeout-failure-taxonomyfrom
timeout-stuck-message-reaper
Open

Clear AI messages abandoned in processing#5071
elias-ba wants to merge 1 commit into
timeout-failure-taxonomyfrom
timeout-stuck-message-reaper

Conversation

@elias-ba

@elias-ba elias-ba commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Description

processing_started_at has been written on every message since it started processing, and read by nothing. This reads it.

A message is left processing forever when its job dies without emitting telemetry. Attaching Oban's stop event in #5069 covers most of that, but not the case it cannot reach: when a job outlives the drain window, Oban stops the producer first and kills the task after, so there is nothing left to report it. Until something clears the row the panel stays locked for everyone in that session, and nothing raises.

Two guards decide what to reap, because neither is sound alone. Age is vulnerable to clock skew and to a queue that was paused. A live Oban job for the same message is the cross-node authority. The live set is read after the candidates, never before: a job starting in between then appears in it and is skipped, where the other order could reap a message whose job had only just begun.

The write is a guarded update rather than a read-modify-write, so a job that finishes between the select and the write keeps its own result. The grace period sits well clear of the longest a run can legitimately take, so a slow answer is never mistaken for an abandoned one, and a partial index keeps the every-five-minutes scan off the table. The sweep is batched at 200, since each message reaped loads its whole session to tell the panel and the backlog this exists for is exactly when that would be largest. Oldest go first and the rest wait for the next pass.

Reaping something also meant telling the panel which message it was about, which is the second half of this PR.

The broadcast now carries the message id. Without it a listener has to guess, and it guessed by taking the newest, so clearing a message stranded several exchanges back marked an unrelated later message as failed. broadcast_status/3 carries the id, broadcast_message_error/2 takes it, and the channel resolves it. Broadcasts that do not carry an id keep the old guess, so nothing else changes behaviour.

Closes #4260

Validation steps

  1. Leave a message stranded, as if its job died without saying so. In IEx:
    import Ecto.Query
    msg = Lightning.Repo.one(from m in Lightning.AiAssistant.ChatMessage,
      order_by: [desc: m.inserted_at], limit: 1)
    Lightning.Repo.update_all(
      from(m in Lightning.AiAssistant.ChatMessage, where: m.id == ^msg.id),
      set: [status: :processing,
            processing_started_at: DateTime.add(DateTime.utc_now(), -3600)]
    )
    With that session open in the browser, the panel sits on "...".
  2. Run the sweep by hand: Lightning.AiAssistant.StuckMessageReaper.perform(%Oban.Job{}) The panel should unlock and the message show as failed.
  3. For the second half, do the same in a session that has a later successful exchange. Only the stranded message should be marked failed; the newer one should be left alone. That is what the id on the broadcast is for.

Additional notes for the reviewer

The second half changes which message the client is told failed. Broadcasts carrying no id keep the old "newest user message" guess, so only the reaper's path behaves differently.

AI Usage

Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):

  • I have used Claude Code
  • I have used another model
  • I have not used AI

You can read more details in our Responsible AI Policy

Pre-submission checklist

  • I have performed an AI review of my code (we recommend using /review with Claude Code)
  • I have implemented and tested all related authorization policies. (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog
  • I have ticked a box in "AI usage" in this PR

@github-project-automation github-project-automation Bot moved this to New Issues in Core Aug 15, 2026
@github-actions

Copy link
Copy Markdown

Security Review ✅

  • S0 (project scoping): Reaper is a system-wide background worker, not a user-facing endpoint; its broadcast targets ai_session:{id}, joined only after authorize_session_access in lib/lightning_web/channels/ai_assistant_channel.ex:39.
  • S1 (authorization): N/A — no new user-facing actions; changes are internal to Oban worker/telemetry callbacks and to broadcasts already gated at channel join.
  • S2 (audit trail): N/A — writes touch only chat message status/failure metadata (ai_chat_messages), not project/instance configuration resources.

@elias-ba
elias-ba force-pushed the timeout-failure-taxonomy branch from 3683332 to 5a28e60 Compare August 15, 2026 22:50
@elias-ba
elias-ba force-pushed the timeout-stuck-message-reaper branch from 01753c3 to 55042f3 Compare August 15, 2026 22:50
@elias-ba
elias-ba force-pushed the timeout-failure-taxonomy branch from 5a28e60 to 1813938 Compare August 15, 2026 23:40
@elias-ba
elias-ba force-pushed the timeout-stuck-message-reaper branch from 55042f3 to 3abf115 Compare August 15, 2026 23:40
@elias-ba
elias-ba force-pushed the timeout-failure-taxonomy branch from 1813938 to d16eb79 Compare August 17, 2026 02:53
@elias-ba
elias-ba force-pushed the timeout-stuck-message-reaper branch from 3abf115 to d0b1213 Compare August 17, 2026 02:53
@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.32258% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.5%. Comparing base (e815b6a) to head (43d8b43).

Files with missing lines Patch % Lines
lib/lightning_web/channels/ai_assistant_channel.ex 81.8% 2 Missing ⚠️
lib/lightning/ai_assistant/message_processor.ex 83.3% 1 Missing ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           timeout-failure-taxonomy   #5071     +/-   ##
==========================================================
- Coverage                      90.5%   90.5%   -0.0%     
==========================================================
  Files                           421     422      +1     
  Lines                         20024   20044     +20     
==========================================================
+ Hits                          18131   18148     +17     
- Misses                         1893    1896      +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

processing_started_at has been written on every message since it started
processing and read by nothing. This reads it.

A message is left processing forever when its job dies without emitting
telemetry. Attaching Oban's stop event covers most of that, but not the
case it cannot reach: when a job outlives the drain window, Oban stops
the producer first and kills the task after, so there is nothing left to
report it. Until something clears the row the panel stays locked for
everyone in that session, and nothing raises.

Two guards decide what to reap, because neither is sound alone. Age is
vulnerable to clock skew and to a queue that was paused. A live Oban job
for the same message is the cross-node authority. The live set is read
after the candidates, never before - a job starting in between then
appears in it and is skipped, where the other order could reap a message
whose job had only just begun.

The write is a guarded update rather than a read-modify-write, so a job
that finishes between the select and the write keeps its own result.

The grace period sits well clear of the longest a run can legitimately
take, so a slow answer is never mistaken for an abandoned one, and a
partial index keeps the every-five-minutes scan off the table.

The sweep is batched. Each message reaped loads its whole session to tell
the panel, and the backlog this worker exists for is exactly when that
would be largest; the oldest go first and the rest wait five minutes.

The broadcast now carries the message id. Without it a listener has to
guess, and it guessed by taking the newest - so clearing something
stranded several exchanges back marked an unrelated later message as
failed. The channel uses the id when it is given one and keeps the old
guess only for broadcasts that do not carry it.
@elias-ba
elias-ba force-pushed the timeout-failure-taxonomy branch from d340e01 to e815b6a Compare August 18, 2026 07:29
@elias-ba
elias-ba force-pushed the timeout-stuck-message-reaper branch from abe0cd6 to 43d8b43 Compare August 18, 2026 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New Issues

Development

Successfully merging this pull request may close these issues.

1 participant