Skip to content

Keep the status updates a dying stream already showed - #5074

Open
elias-ba wants to merge 4 commits into
timeout-separate-dialsfrom
timeout-keep-status-timeline
Open

Keep the status updates a dying stream already showed#5074
elias-ba wants to merge 4 commits into
timeout-separate-dialsfrom
timeout-keep-status-timeline

Conversation

@elias-ba

Copy link
Copy Markdown
Contributor

Description

The partial save kept the text and the workflow YAML but dropped the status timeline, so a failed workflow chat reloaded missing the steps the user had watched go by.

Text is now closed off at each status update, so the saved timeline reads in the order it appeared. A reply with no status in it still saves flat, since that renders the same from the content alone and changing it would alter how every job chat failure looks.

The tests here drive a real socket rather than the Tesla mock, because none of this behaviour exists until something walks the lazy body stream. They cover job, workflow and global chat.

Closes #__

Validation steps

  1. mix test test/lightning/ai_assistant/stream_failure_test.exs
  2. Comment out the Map.put(attrs, :response_segments, segments) line and check the timeline test fails.

AI Usage

  • I have used Claude Code

Pre-submission checklist

  • I have performed an AI review of my code
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

@github-actions

Copy link
Copy Markdown

Security Review ✅

  • S0 (project scoping): No new project-scoped queries or web entrypoints were introduced; the reaper's system-wide scan of :processing messages is a background sweep (like Lightning.Janitor), and channel broadcasts still ride the already-authorized ai_session:{session_id} topic set up in lib/lightning_web/channels/ai_assistant_channel.ex:43.
  • S1 (authorization): N/A — no new handle_in, handle_event, or controller actions; the added retry_message/streaming paths reuse existing project-access checks and Limiter.validate_quota/1.
  • S2 (audit trail): N/A — chat messages are user-generated conversation content, not project/instance configuration (workflows, credentials, project settings, webhook auth methods, OAuth clients, version control), so no audit entry is expected for the new failure_category/failure_message writes.

@elias-ba
elias-ba force-pushed the timeout-separate-dials branch from aacaf77 to 4428533 Compare August 15, 2026 22:50
@elias-ba
elias-ba force-pushed the timeout-keep-status-timeline branch from 909036d to bda6aac Compare August 15, 2026 22:50
The partial save kept the text and the workflow YAML but dropped the
status timeline, so a failed workflow chat reloaded missing the steps the
user had watched go by.

Text is closed off at each status update, so the saved timeline is in the
order it appeared. A reply with no status in it still saves flat, since
that renders the same from the content alone.
Covers the whole stack, including the APOLLO_TIMEOUT replacement, which
deployments have to act on.
@elias-ba
elias-ba force-pushed the timeout-separate-dials branch from 4428533 to c7821e5 Compare August 15, 2026 23:40
@elias-ba
elias-ba force-pushed the timeout-keep-status-timeline branch from bda6aac to 261a6ff Compare August 15, 2026 23:40
Nine things, all from reviewing the corrected stack rather than the
original diff. They span every PR in it, so they land here rather than in
another six-deep rebase.

The timeout message was unreachable. Finch raises its own
%Mint.TransportError{reason: :timeout} from receive_timeout inside its
streaming task, and usually wins the race against the adapter's own after
clause, so a hung Apollo read as "the connection was lost" - the exact
conflation the stack exists to end. Both now say the same thing.

Apollo's error text no longer reaches the panel or the row. Apollo wraps
any unhandled exception as str(e), which carries internal hostnames,
upstream URLs and container paths. That it failed is worth recording,
because it separates a service giving up from a socket going quiet; its
words are not.

A partial save in a global chat lost from_global, so it was treated as job
code and had the session's job attached - the workflow YAML came back
rendered as a code suggestion, in the chat type where keeping it matters
most.

An empty status segment invalidated the whole changeset, discarding the
text too. The complete path drops invalid segments rather than carrying
them; this one now does the same, and the comment claiming they already
matched is gone.

The reaper told the panel a session had failed but not which message, and
the panel assumed the newest - so clearing something stranded several
exchanges back marked an unrelated success as failed. The id travels with
the broadcast now.

The partial was saved and never sent, so a user watched their answer be
replaced by an error and only found it again by reloading.

The drain window is derived from the same three settings the job ceiling
is, instead of being fixed at six minutes. Dev's own defaults exceeded it,
so the warning added in this stack fired on every boot, advising a change
to something no variable could reach. OBAN_SHUTDOWN_GRACE_PERIOD_MS now
can.

Also: failure_message is bounded, since it is written through change/2
which validates nothing and is re-sent on every join; one internal failure
no longer logs three times, which was three Sentry events; the reaper's
sweep is batched, since each message reaped loads its whole session and
the backlog it exists for is when that is largest; and the pool comment no
longer implies http1 is being changed when it is already Finch's default.
A turn that failed before producing anything re-sent the partial kept from
an earlier turn, so the client showed it twice. The last message in the
session is the right one to look at, not the newest assistant one.

Dropping empty segments missed whitespace-only ones, and validate_required
trims before it checks - so a status update of spaces still invalidated the
embed and took the text down with it.
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