fix(dashboard): a failing slots broadcast must not fail the write

[chenmingwei23]

Problem / Motivation

You click New chat. The chat is created. You are told it failed.

api_chat_slot_create finishes its work, so the slot is in state._slots and its
metadata is on disk. Only then does suspend_slots_push.__exit__ flush the owed
slots push. On the coalescing window's leading edge that flush broadcasts right
away, so an exception in it escaped the handler and aiohttp answered a text/plain
500. The write had already happened. You retry, and now you have two sessions, or
a name conflict on a session you cannot see.

Inside the 0.2s window (_SLOTS_BROADCAST_INTERVAL_S) the very same fault was
deferred through loop.call_later(_trailing_slots_flush), the handler answered
200, and the exception turned up later as a log line. So a timer decided what the
caller was told. A busy dashboard got 200 and a log; an idle one -- someone doing
a single deliberate New chat -- got the 500. No client can be written against
that, and the failure landed on the most deliberate case.

Reachability, stated plainly: no production path is demonstrated. The evidenced
fault is a value in slot state that json.dumps cannot serialize, and a value
parsed from a request body is serializable by construction, so this needs a
server-constructed object to reach slot state. In #6522 that was a test fixture.
This is latent, not live.

Why it matters

A caller cannot tell "it did not happen" from "it happened and I could not tell
you". Those need opposite responses. The first says retry. The second says do not.

The broadcast serializes EVERY slot. The response serializes one. So one bad
value on some unrelated old session failed a brand-new, perfectly healthy create.
The write was blamed for a fault in the thing announcing it.

And the 500 was not even the honest error. A broken slots projection breaks every
slots read path too -- GET /api/chat/slots, the websocket connect snapshot. The
dashboard is unrenderable whether or not anyone created a slot. The create was
just standing nearby when the projection broke.

What changed (motivation -> approach -> change)

The symptom is a 500 on a create that committed. The root cause is that work
which runs AFTER a commit could fail that commit. A broadcast is that kind of
work. A listener being absent, slow or broken says nothing about whether the
write landed, so it must not be able to fail the write.

So the broadcast's failure is contained, and contained in one place:
_do_slots_broadcast. That is the single funnel both coalescing branches reach --
the leading-edge call in push_slots_update and the trailing timer's
_trailing_slots_flush. Containing it there makes both branches answer the same
way by construction, which is the convergence #8745 asks for. No timer decides
anything any more. The frame-building body moved unchanged into
_send_slots_frames; raising is still how it reports a fault, and the guard is
the only thing that reads that.

flowchart LR
  subgraph Before
    A1[create commits]:::ctx --> B1[flush broadcasts]:::ctx --> C1[dumps raises]:::removed --> D1[HTTP 500]:::removed
  end
  subgraph After
    A2[create commits]:::ctx --> B2[flush broadcasts]:::ctx --> C2[dumps raises]:::changed --> D2[log + count]:::added --> E2[HTTP 200]:::added
  end
  classDef added fill:#DCFCE7,stroke:#16A34A,color:#14532D,stroke-width:2px
  classDef changed fill:#FEF3C7,stroke:#D97706,color:#78350F,stroke-width:2px
  classDef removed fill:#FEE2E2,stroke:#DC2626,color:#7F1D1D,stroke-dasharray:4 3
  classDef ctx fill:#E0F2FE,stroke:#0284C7,color:#0C4A6E
  linkStyle 2 stroke:#DC2626,stroke-dasharray:4 3
  linkStyle 4,5 stroke:#16A34A,stroke-width:2px

Loading

Legend: green added, amber changed, red removed, blue unchanged.
The failing broadcast now ends in a log line instead of in the caller's response.

Contained is not silent. Every drop increments _slots_broadcast_drops. The
first one logs a full traceback, and that traceback carries the offender note
#8888 added, which names the slot, the field and the type. Only repeats are
throttled, to one line per _SLOTS_BROADCAST_DROP_LOG_INTERVAL_S, and each line
carries the running count. The throttle is there because the fault is a property
of slot state, not of one push: it recurs on every later broadcast, and a slot
mutation fires several per turn, so logging each one buries the first traceback
under thousands of copies.

The slots READ paths keep failing loud, and that is the point, not an oversight.
A read path's answer IS the projection, so a caller asking for it must be told it
is unrenderable. This path's answer is "something changed", and the change is
already true whether or not anyone hears it. Same fault, opposite obligation.

Both counters are class-level defaults, following the block at the top of
DashboardState. The failure handler runs on a __new__-built state too, and an
__init__-only attribute would raise AttributeError there -- re-raising the
very 500 this exists to absorb, and only in those test suites.

Which side of the commit each step is on

Read this to check that everything made non-fatal is genuinely after the commit.

1. body read, folder/mode/memory validation, the three remote-binding gates -- before
2. create_peer_slot (remote binds only) -- before
3. get_or_create_slot -- THE COMMIT: the slot is in state._slots
4. title, artifact, folder filing, folder-tag inheritance, project defaults -- after
5. save_slot_off_loop(force=True) -- after, already contained: best_effort=True logs and marks the slot dirty, never raises
6. suspend_slots_push.__exit__ -> the slots broadcast -- after, THIS is what 500'd, and what this PR contains
7. schedule_eager_spawn -- after, already self-contained: its config read is wrapped in except Exception: return None
8. return web.json_response(state.serialize_slot(slot)) -- after, still exposed

The other post-commit step, named rather than left for a reviewer to find

Step 8 still turns a raise into a 500 on a committed create, and this PR leaves it
alone on purpose.

It serializes only the created slot, where the broadcast serializes all of them.
For the evidenced fault -- a bad value on some OTHER slot -- step 6 fails and step
8 does not, which is exactly the case that made a healthy create 500. That case is
now closed. The remaining case is a bad value on the NEW slot itself, and there the
caller genuinely cannot be handed the payload it asked for. Deciding what to answer
instead is a different question from containment, and widening this PR to guess at
it would ship an unreviewed contract.

Step 7 is structurally the same shape and is already contained, so it is listed
above for completeness rather than as work.

One prose change comes with the behaviour change. suspend_slots_push's PEP 678
annotation exists for a flush that fails while the body's own exception is
unwinding, and the evidenced way to reach it is now gone -- a contained broadcast
does not raise, so the body's fault propagates as itself instead of being demoted
to __context__. The annotation stays, because the rest of push_slots_update
(a lock or timer-scheduling fault) can still reach it. The docstring now says
which case it is left for.

Tests

test/test_slot_create_broadcast_failure.py, new, is the handler-visible half.
It POSTs /api/chat/slots with the broadcast broken and pins that the response
is a success, that the slot really is in state._slots, that the drop was counted
and that it was logged. A second test pins that the response still describes the
new slot, because a 200 with an empty body would be its own defect -- the
dashboard renders the created session from that payload. The third is the benign
control: a healthy create drops nothing and logs nothing. It breaks _broadcast
rather than serialize_slots, because poisoning the shared projection would break
the response too and the test would pass for the wrong reason.

In test/test_open_slots_persistence.py, the four pins that #8888 wrote for the
old fail-loud semantics now pin containment instead. That file's #8745 section
said its diagnostics were "independent of whichever option above is chosen", and
this PR chooses one, so those propagation assertions are the part that moves.
What they protected is kept: the leading-edge test asserts the offender note
survives containment by reading it off the logged record's exception, so the note
cannot be lost. Added alongside: both timing branches contained, every drop
counted with repeats throttled to one line, the throttle resuming afterwards with
the running count, containment working on a __new__-built state, and the body's
own exception no longer being masked by its flush.

In test/test_dashboard_state_ws.py, two comments reading "must not raise" now
say what actually pins those tests -- the frame assertion, since call_args is
None when no frame is sent. Containment would otherwise have made that comment
describe something the guard now provides for free.

Scoped runs, all green: test_open_slots_persistence.py 77 passed,
test_slot_create_broadcast_failure.py 3 passed, test_dashboard_state_ws.py 80
passed (that last file is a consumer -- it drives _do_slots_broadcast directly
seven times). black, isort, flake8 clean on the changed files; mypy reports
nothing in state.py. The two touched test files are in the black baseline and
were deliberately left unformatted so the shrinking baseline is not tripped; only
the new lines were shaped to black's output by hand.

Mutation-verified rather than asserted: reverting the production hunks while
keeping the test hunks reddens the new tests on their own assertion messages.

Manual verification

N/A -- unit coverage sufficient. The fault has no demonstrated production trigger
(a live gateway journal over three days shows zero frames through this broadcast),
so reproducing it by hand means constructing the poisoned value the tests already
construct, at the same seam.

Related Issues

Closes #8745

Pattern harvest

The defect class is failure attribution across a commit boundary: a post-commit
notification whose exception propagates into the committing request, so the caller
is told the write failed when it succeeded. It has a recognisable shape --
announce/notify/broadcast/publish work reached from a context manager's __exit__
or from the tail of a handler, after the state change it describes. It also has a
recognisable tell: two code paths for one fault, one synchronous and one deferred,
which makes the caller-visible outcome depend on a timer rather than on the fault.

Rule candidate: review-prompt
Pattern: a broadcast or notification raising after its write has committed, so a
committed operation is reported to the caller as failed.

[chenmingwei23]

Intent: Make a slots broadcast unable to fail the write it announces, so a slot create that committed is never reported to the caller as a 500, and so both coalescing branches (leading edge and trailing timer) answer a broadcast fault identically instead of a 0.2s timer deciding.

Not a goal: Changing the slots READ paths, which correctly still fail loud because their answer IS the projection. Not deciding what the create response should say when the NEW slot itself is unserializable (post-commit step 8 in the PR body). Not validating slot state at write time (option 3 in the issue), which can reject writes that succeed today. Not removing the PEP 678 annotation in suspend_slots_push, which still covers a non-broadcast flush fault.

[github-actions]

Design Review (Fable 5.1) — ✅ PASS

Design-level review of 4c85f89a572b41629eabde23bc6fa5c06e3e5b6e — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Correctly separates a write's commit from its announcement: a broadcast that fails to notify listeners must not fail the create it already committed, while read paths still fail loud.

[DESIGN-REVIEWED] 4c85f89

[github-actions]

First Principles Review (Fable 5.1) — ✅ PASS

Premise-level review of 4c85f89a572b41629eabde23bc6fa5c06e3e5b6e — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Everything checks out. The fix sits at cause level (contain post-commit work at the single funnel both branches reach), the counter/throttle each have a named harm, and no shared throttle helper exists to reuse. Writing the review.

First-Principles-Verdict: PASS

Contains a post-commit broadcast failure at the one funnel both coalescing branches reach; every added element (counter, throttle) has a named harm and a real consumer.

What this change ships

Intent: stop a slots-broadcast serialization fault on an unrelated slot from 500ing a create that already committed. This is a FIX.

1. A failing slots broadcast is caught and logged instead of raising to the caller — justified (removes the reported 500-on-committed-write).
2. Frame-building body extracted into _send_slots_frames, called only by _do_slots_broadcast — justified (the extraction is what lets the guard wrap it).
3. New _slots_broadcast_drops counter (class-level default) — justified (1 consumer: the throttle/log branch at state.py:7886–7897; class-level is mandated because the handler runs on __new__-built state).
4. Repeat-log throttle via _SLOTS_BROADCAST_DROP_LOG_INTERVAL_S + _slots_broadcast_drop_logged — justified (named harm: fault recurs on every broadcast, several per turn, would bury the first traceback).
5. Docstring/comment/test-name updates reflecting containment — supporting.

No duplicate mechanism: the throttled-log pattern recurs (_WORKING_LOG_INTERVAL_SECS in acp/session_handle.py, session_pid_sig.py) but as local ad-hoc counters, with no shared helper to reuse — not a second spelling worth flagging. Step 8 (the response's own serialize_slot) is a named, in-scope-excluded sibling with a genuinely different contract (the caller asked for that payload), not an unfixed point-patch. Zero option of the counter/throttle is not costless (silent swallow or log flood), so nothing rides along free.

[FIRST-PRINCIPLES-REVIEWED] 4c85f89

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 4c85f89a572b41629eabde23bc6fa5c06e3e5b6e and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 4c85f89

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 4c85f89a572b41629eabde23bc6fa5c06e3e5b6e: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 4c85f89a572b41629eabde23bc6fa5c06e3e5b6e — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 4c85f89

Verdict parsed from the review's SHA-scoped output markers for commit 4c85f89a572b41629eabde23bc6fa5c06e3e5b6e.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 4c85f89a572b41629eabde23bc6fa5c06e3e5b6e: <one-sentence reason>

[chenmingwei23]

Closing unmerged. The item is stood down, and the analysis is on the issue.

Two reasons, and the second one is on the pipeline rather than on this branch.

The durable-write half of #8745 is already correct on main and pinned by two named tests in
TestDurableWriteOrdering, so there was nothing to fix there. The status-code half -- whether an
already-committed create should answer success rather than 500 -- is the half of #6532 that was
declined, and the test at test/test_chat_slot_create_folder.py carries an in-tree comment saying
so directly above the assertion, warning that folding it in resurrects a declined decision.

This branch's state.py change makes that assertion fail, which is exactly the contract doing its
job. The pipeline's own instruction sent the work at that decision without knowing it existed, so
the branch is being withdrawn rather than argued for. The tests were not touched and should not be.

Worth recording: this diff is what demonstrated the contract is real and load-bearing rather than
incidental, which is a useful outcome even though nothing ships from it.