Skip to content

fix: persist the deferred-note hold with the slot so it survives a restart (#4093) - #8982

Merged
iamwhatever merged 1 commit into
mainfrom
fix/persist-deferred-note-hold-4093
Sep 7, 2026
Merged

fix: persist the deferred-note hold with the slot so it survives a restart (#4093)#8982
iamwhatever merged 1 commit into
mainfrom
fix/persist-deferred-note-hold-4093

Conversation

@NicholasRBowers

@NicholasRBowers NicholasRBowers commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

POST /api/chat/slots/{slot}/note accepts a note while a turn is running and replies 200 with visibleDeferred: true — a delivery promise for a transcript line. Both halves of the held note live in memory only: the visible line in _ChatSlot._deferred_notes and its queued context entry embedded in it. The persistence layer references neither (the cleanup-path comment in chat_handlers.py said so explicitly), so a gateway restart between the 200 and the next turn silently drops the note. The only mitigation was documentation: "a 200 means accepted for this gateway lifetime".

Why it matters

Background actors (crons, apps) are exactly the callers that post notes into running turns, and a gateway restart happens on every upgrade and every crash. A caller that got a 200 has no signal the note evaporated; the user never sees the line, and the "re-post it yourself" advice in the docs is unactionable for a caller that no longer exists. The response field reads as a delivery promise and the system did not keep it.

What changed (motivation → approach → change)

Symptom: an acknowledged held note vanishes across a restart. Root cause: the hold has no durable representation. Fix: persist the hold with the slot, under one invariant — retirement is row-derived. Every note carries an id; the flush stamps each delivered inject row with it (meta.noteId) and records rebind-dropped ids; the full save retires exactly the entries whose rows are in the window it writes (or whose ids were dropped) and keeps everything else:

  • Durable before the 200 (chat_handlers.py): the /note deferred branch persists the hold via asyncio.to_thread + update_metadata_if, with everything read inside the guard the store evaluates under the cross-process history lock (the established SLOT_OWNED_META_KEYS read-modify-write pattern; never an outside-lock snapshot). The write is a merge (union_deferred_notes): live entries plus disk entries whose note id is absent from memory — absent-from-memory can mean "delivered into the still-unsaved window", and erasing such an entry would lose an acknowledged note on crash. The posted note is pinned into the write (ensure=) so a turn-end flush draining it mid-persist cannot yield a 200 with no durable copy anywhere — and the pin covers exactly one state (not held, not durable, not dropped, not committed): under the lock it is skipped when the note's delivered row is already in the committed transcript, so a late worker cannot resurrect a retired hold into a duplicate replay. The union never evicts: at the 2× ceiling the new note is refused (429 deferred_notes_full) rather than a retained entry dropped. A failed write — including an unreadable metadata record, which update_metadata_if reports identically to an absent one — rolls the note back by identity (equality could evict a byte-identical sibling holding a durable 200) and answers a retryable 503 deferred_note_persist_failed. A slot with no metadata line at all keeps its 200 (it has no durable identity for the hold to outlive).
  • Retired by the row-committing save, row-derived (chat_persistence.py, history.py, slot_buffers.py): deferred_notes joins SLOT_OWNED_META_KEYS, and the full save — reading the on-disk and live holds under the same history lock the merge writers commit under — retires exactly the entries whose delivered rows (stamped meta.noteId by the flush) are in the window this save writes, plus recorded rebind-drops, and unions everything else forward. Row and retirement land in one atomic file replace, so no flush/save/enqueue interleaving can clear an entry whose row is unsaved, and a /note commit that wins the lock during the save's patient acquire is kept rather than overwritten by a stale snapshot. The flush itself writes no metadata at all: a crash between flush and save re-delivers on restore — at-least-once, the correct failure direction for a delivery promise.
  • Replayed on restore, up to the durable ceiling (chat_persistence.py): both slot-restore paths read the persisted hold back into _deferred_notes, so the existing flush_deferred_notes() call sites deliver it on the first turn after the restart — the flush sites themselves are unchanged. The restore is bounded by _MAX_DURABLE_HOLD_ENTRIES (2× the live cap), the same ceiling the persist path admits: every durable entry is a 200-acknowledged note whose caller was told not to re-post, so a live-cap restore would silently discard acknowledged content. The live cap still binds new enqueues, and the first flush drains the surplus. Restored notes are sanitized fail-closed: a note without an authorization session stamp is dropped (the cross-session leak the stamp prevents), a context half that fails the pending-context schema is dropped alone so a corrupted entry cannot poison the flush or the next turn's drain.
  • Verbatim durable copy, bounded at the enqueue boundary (slot_buffers.py, chat_handlers.py): a note posted during a running turn is capped at 4,000 chars with an actionable 413 deferred_note_too_large before any 200 — and the durable copy is then persisted verbatim, never truncated, so a restart replays exactly what the 200 accepted. Restore drops (never alters) an over-bound entry as tamper evidence. The boundary rejection is also what keeps the metadata line small (it is read and rewritten whole under the history lock on every /note POST).
  • Docs updated in the same commit: the App Kit API reference paragraph, the session.md handover rationale, and the endpoint docstring now state the new contract — do not re-post after a restart (the restored hold delivers, and a re-post would double the line); 503 = retry the same request, 413/429 = boundary refusals.

_pending_context (the /context endpoint's queue) deliberately stays memory-only, as the issue allows: only the context halves embedded in held notes ride the metadata shape naturally. (Sibling PR #6813 addresses the context queue across a close; no overlap with this diff.)

The rows-only handover save defers this key to the on-disk value (clearing it there could erase a live replacement slot's own hold); the restore closes the resulting window by dropping any restored entry whose delivered row is already committed in the transcript (drop_committed_restored_notes), failing toward a duplicate rather than a loss when the transcript is unreadable.

Tests

test/test_deferred_note_persistence.py (38 tests), pinning the issue's regression gates:

  • (a) round trip: persist → fresh slot restore → first flush delivers exactly one copy → the save retires the hold → a second restart re-delivers nothing and the row is on disk.
  • (b) save-owned retirement: the flush does not clear the durable copy (crash between flush and save re-delivers instead of losing — pinned by its own test); the full save clears by absence; the enqueue merge retains a delivered-but-unsaved entry alongside a racing new note and the save then retires exactly the delivered one; the empty-window merge save unions instead of shrinking; ensure= pins a note a racing flush already drained; the hold-full ceiling refuses the new note without evicting a retained one.
  • (c) durable-before-200: after a 200 the hold is on disk (id, session stamp, context); a failed write rolls back by identity (byte-identical sibling preserved) and answers 503; an unreadable record raises instead of reading as "no durable identity"; an oversized deferred note gets a 413 before any 200; a slot with no metadata line and a memory-only state keep prior semantics.
  • (e) evidence rules (round 9b): a recorded drop dominates durable evidence (a drop-marked sibling entry cannot back a 200 — it is retired row-lessly); a flush-drop under a matching history key refuses instead of acknowledging a written merge that carries no representation of the note; a delivered live row keeps the 200 when the writer itself fails; a committed-filtered restore entry is recorded for row-less retirement and the next full save actually removes it (no permanent hold-slot leak).
  • (d) trust boundary: restore drops notes without a session stamp, drops (never truncates) over-bound content, validates the context half field-by-field, caps at MAX_DEFERRED_NOTES, mints ids for legacy entries; the serializer is verbatim.

Existing coverage unchanged: the deferred-note endpoint suites, flush-exception-safety, slot facade contract, restore/rehydrate, history/atomic-rewrite, and slot-close recreation-race suites all pass (968 tests in the touched neighborhood); the full backend suite delta vs pristine main is zero.

Manual verification

N/A — unit coverage exercises the real ConversationLog on-disk round trip end to end (endpoint → metadata line → fresh-state restore → flush → save), which is the integration surface this change touches.

Related Issues

Closes #4093

Pattern harvest

Rule candidate: review-prompt
Pattern: an acknowledgement's backing state cleared by a non-committing writer — any "clear on consume" of a durability record must ride the same atomic write that commits the consumed data, never an independent earlier write.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound durable-before-ack design with row-derived retirement; the retrofitted 4,000-char cap is a timing-dependent contract regression existing callers will hit.

Watch

  • The new 413 shrinks the accepted size of a mid-turn note ~10× (from the 40,000-char shared bound to MAX_DEFERRED_NOTE_CHARS = 4000), and it fires only when a turn happens to be running at POST time — a caller cannot predict a hold, so an existing app/cron posting >4,000-char notes now fails nondeterministically where it used to get a 200 and delivery. The effective contract is "every note ≤4,000"; either state that plainly in the API reference or store the durable copy outside the whole-line metadata rewrite so the old bound can stand.
  • The version-skew hole session.md documents — an older gateway stamps no meta.noteId, so a downgrade-deliver-reupgrade cycle replays delivered notes as duplicates — is the accepted at-least-once direction, but it is invisible to operators; worth a line in the release notes when this ships.

Suggestions

  • note_hold_durable and _note_row_committed in slot_buffers.py reach into ConversationLog privates (_read_metadata, _path); expose a public read/probe on the log so its file layout doesn't become a cross-module dependency.

[DESIGN-REVIEWED] 0749128

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 0749128d294de89cb4c83cf56beb056dbeda0262 — 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.

All verification complete. The fix reuses the existing SLOT_OWNED_META_KEYS/update_metadata_if mechanism rather than inventing a store, _disk_meta_observed pre-exists in state.py, the one root-cause sibling (_pending_context) is declared and deferred to a named PR, and no pre-existing durable queue could have held the note. Final review:

First-Principles-Verdict: PASS

A 200 that promised delivery now has a durable owner on every path; each edge refusal traces to a named loss-or-duplicate failure.

What this change ships

Intent: a note acknowledged with 200 mid-turn must still reach the transcript after a gateway restart. FIX (issue #4093; the removed doc/comment text explicitly recorded the gap).

  1. A held note now survives a restart and delivers on the first turn after — justified (the fix, cause level: the hold had no durable representation)
  2. Callers must no longer re-post after a restart (docs/docstring contract flip) — justified, the fix's point
  3. New 413: deferred notes capped at 4,000 chars (was 40,000 shared bound) — declared, derived (metadata line rewritten whole under the cross-process lock per POST)
  4. New 503 deferred_note_persist_failed retry signal — justified (durable-before-200)
  5. 429 deferred_notes_full now also fires at the 2× durable ceiling — declared, refuses-new over evicting an acknowledged copy
  6. New 404 when a rebind/flush-drop voids the note mid-persist (was silent drop after a 200) — justified (app-isolation boundary, named cause)
  7. Persisted deferred_notes metadata key + meta.noteId row stamp — justified; reuses existing SLOT_OWNED_META_KEYS mechanism (10 occurrences, 3 files)
  8. Held-note context (unredacted trusted channel) now lives on disk until retirement — declared retention consequence
  9. New slot state _dropped_note_ids for row-less retirement — justified (dropped rows never exist)
  10. _pending_context stays memory-only — declared, deferred to sibling PR fix(chat): persist undrained pending context across a close #6813

Subtractions

  • Drop the _MAX_DEFERRED_NOTES = MAX_DEFERRED_NOTES alias in chat_handlers.py:9507 — a second spelling of the same constant; its 2 code consumers (lines 10279, 10282; grepped _MAX_DEFERRED_NOTES) can use the imported MAX_DEFERRED_NOTES directly.

[FIRST-PRINCIPLES-REVIEWED] 0749128

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 0749128d294de89cb4c83cf56beb056dbeda0262 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0749128

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

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 0749128d294de89cb4c83cf56beb056dbeda0262 — this comment is updated in place on each push.

Review details

I've independently verified the two most plausible concrete-defect spots: _note_row_committed's note_id not in line operates on a decoded str (bounded_records yields _decode(raw)), so no str/bytes TypeError; and the rows-only save path correctly defers deferred_notes to the on-disk value (deferred_notes ∈ SLOT_OWNED_META_KEYS - ROWS_ONLY_OWNED_META_KEYS = ROWS_ONLY_DEFERRED_META_KEYS, popped and carried), consistent with retired_drop_ids = set() there. Both check out. No candidate survived, and I could not ground a new finding to the required bar.

No findings.

[OPUS-REVIEWED] 0749128

Verdict parsed from the review's SHA-scoped output markers for commit 0749128d294de89cb4c83cf56beb056dbeda0262.

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

@NicholasRBowers
NicholasRBowers force-pushed the fix/persist-deferred-note-hold-4093 branch from b612da6 to 6d0e1e0 Compare September 6, 2026 10:43
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=1ce50744f47d (chat_persistence.py:3199, torn save retires a note without committing its row)

Fixed in 6d0e1e0: the full save now snapshots the deferred hold INSIDE the same bounded-retry block as the window snapshot (recheck detects a flush interleaving between the two and retries; the exhausted-retries fallback reads the hold before the window, biasing any residual tear toward duplicate-on-replay, never loss). The meta build serializes that paired snapshot instead of re-reading live state. The empty-window merge save got the same treatment via the shared union writer.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=e86f516189bb (slot_buffers.py:206, persistence can 200 a note it never writes)

Fixed in 6d0e1e0: persist_deferred_notes_sync now takes ensure= — the handler passes the exact posted note, and the lock-time guard pins that note's serialized entry into the written union even when a concurrent turn-end flush drained it from the live hold first. A 200 can no longer race a flush into acknowledging a note with no durable copy anywhere. Pinned by test_ensure_pins_a_note_a_racing_flush_already_drained.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=e86f516189bb (slot_buffers.py:220, union cap discards an older acknowledged hold)

Fixed in 6d0e1e0 with the finding's own proposed remedy: the union never truncates retained entries. At the 2x-cap ceiling the NEW note is refused instead — DeferredHoldFull maps to the existing 429 deferred_notes_full and the handler rolls the new note back — because every retained entry is the only durable copy of an already-acknowledged note. Pinned by test_hold_full_refuses_instead_of_evicting (no eviction at the ceiling).

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=e86f516189bb (slot_buffers.py:53, restart replays truncated content for an accepted note)

Fixed in 6d0e1e0 with the finding's second proposed remedy: oversized deferred notes are rejected BEFORE the 200 (413 deferred_note_too_large at 4000 chars, actionable: shorten or wait for the turn to end), and the durable copy is persisted verbatim — the elision path is gone. Restore drops (never truncates) an over-bound entry as tamper evidence. Pinned by test_serialized_hold_is_verbatim and test_oversized_deferred_note_is_rejected_before_the_200.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=d46c2bc8f64f (slot_buffers.py:517, cap eviction of delivered-but-unsaved retained entries)

Fixed in 6d0e1e0, combining both remedies this finding offered: retained entries are never evicted, and the union bound is raised to 2x MAX_DEFERRED_NOTES as a refusal ceiling — crossing it refuses the NEW note (429) rather than dropping a retained one, since reaching it means row-committing saves have not landed for multiple turn cycles. The function's documented shrink invariant now holds unconditionally. Pinned by test_hold_full_refuses_instead_of_evicting.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/persist-deferred-note-hold-4093 branch from 6d0e1e0 to 50c9240 Compare September 6, 2026 11:14
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=e86f516189bb (slot_buffers.py:187, restore discards acknowledged notes above the live cap)

Fixed in 50c9240 with the finding's own remedy: sanitize_restored_deferred_notes now caps at _MAX_DURABLE_HOLD_ENTRIES — the same ceiling the persist path admits — so every durable entry replays and the first flush delivers/retires them; the live cap still binds new enqueues. Pinned by test_restore_replays_every_acknowledged_entry_up_to_the_ceiling (20 restored, 20 delivered, save retires all).

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=1ce50744f47d (chat_persistence.py:2691, stale save erases a newly persisted hold)

Fixed in 50c9240 by making retirement ROW-DERIVED, which supersedes (and removes) the pre-lock snapshot pairing: the flush stamps each delivered row with its note id (meta.noteId), and the save — reading the on-disk and live holds UNDER the history lock via the same union the merge writers use — retires exactly the entries whose rows are in the window it writes (plus recorded rebind-drops) and keeps everything else. A /note commit winning the lock during the save's patient acquire is unioned forward, never overwritten. Pinned by test_save_keeps_an_entry_whose_row_it_does_not_write.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=d46c2bc8f64f (slot_buffers.py:187, restore drops the newest acknowledged deferred notes)

Fixed in 50c9240 exactly as proposed: the restore cap is now _MAX_DURABLE_HOLD_ENTRIES (the persist path's own ceiling), so the newest undelivered 200-acknowledged entries replay instead of being dropped, the first flush delivers them, and the row-committing save retires them. Pinned by test_restore_replays_every_acknowledged_entry_up_to_the_ceiling.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/persist-deferred-note-hold-4093 branch from 50c9240 to ad4ab38 Compare September 6, 2026 12:03
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 6, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=78d6e248f252 (chat_handlers.py:9947, delete before the identity probe still earns a durable 200; 3rd distinct defect on this span)

Fixed in f74f070 with the proposed remedy: had_durable_identity is now seeded from the slot's MONOTONIC _disk_meta_observed witness (set by full saves, both restore paths, and now the durable-hold merge itself) OR'd with the mtime probe, and the locked revalidation stays. A delete landing at ANY point cannot unwind the monotonic bit, which kills the whole probe-timing family rather than this instance. Pinned by test_delete_landing_before_the_probe_is_still_refused (file deleted BEFORE the probe).

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=1ce50744f47d (chat_persistence.py:1053, restore rescans transcripts synchronously on the event loop)

Fixed in f74f070 exactly as proposed: drop_committed_restored_notes is now PURE — it derives the committed noteId set from the message window the restore already loaded off-loop (the prefetched messages at both sites) and never re-opens the transcript or takes the history lock. An absent/empty window keeps every entry (duplicate direction, never loss). Pinned by test_committed_row_filter_is_pure_over_the_loaded_window plus the unchanged end-to-end restore test.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=20d474ef8888 (chat_persistence.py:1053 + :1577 mirror, on-loop transcript rescan; strict-mode OnLoopPersistError silently skipped the dedup)

Fixed in f74f070 with the finding's remedy: the filter scans the already-prefetched message window in memory at both restore sites (the rehydrate site applies it right after the window loads) and never enters _locked or opens the file, so the strict-mode discipline check cannot fire and the dedup cannot be silently skipped. Pinned by test_committed_row_filter_is_pure_over_the_loaded_window and test_restore_drops_an_entry_whose_row_is_already_committed.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • needs-a-decision — span=78d6e248f252 (chat_handlers.py:9975, rebind-dropped note mistaken for delivered; 4th distinct blocking hit on this span)

Per the same-span stall rule this gets a maintainer ruling, not a fifth point patch. The family: every failure branch must infer the note's state (delivered / durable / dropped / still-held) from indirect signals, and each round found one more misread. The reviewer-proposed invariant — answer 200 only on positive evidence (note id present in a live row, committed row, or the durable hold; otherwise return the refusal) — replaces inference with observation and would close the family, but it changes the DeferredHoldRebound contract: a rebind-dropped note would answer 404 where today's code can answer 200. Question for @NicholasRBowers: adopt the positive-evidence rule for all failure branches (my recommendation), or accept the current rollback-miss inference and record the rebind-drop corner as a known residual?

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Intent: Make a 200-acknowledged mid-turn /note survive a gateway restart (issue #4093): persist the held note into the slot's own durable metadata before the 200 is returned, and replay it on slot restore so the first turn after restart delivers it. Failure direction is at-least-once (duplicate over loss).
Not a goal: Persisting _pending_context for the /context endpoint (deferred to #6813); changing the flush call sites; any new sidecar file or persistence mechanism beyond the existing SLOT_OWNED_META_KEYS machinery.

@NicholasRBowers
NicholasRBowers force-pushed the fix/persist-deferred-note-hold-4093 branch from f74f070 to dc1f8eb Compare September 6, 2026 22:58
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • fixed — span=78d6e248f252 (chat_handlers.py:9975, rebind-dropped note mistaken for delivered; resolves the needs-a-decision above per the maintainer's ruling)

Fixed in dc1f8eb by adopting the proposed positive-evidence invariant across ALL failure branches, not a fifth point patch: persist_deferred_notes_sync resolves NoteEvidence (durable-hold entry, committed row) UNDER the lock and carries it on the return value and both hold exceptions; the handler adds the in-memory live-row clause. Every branch answers 200 only on one of those three observations — otherwise the branch's refusal stands, so a rebind-dropped note now gets the 404. The rollback-miss inference is deleted. Pinned by the rebind-drop, hold-full-drained, and channel-origin-twin regression tests (37 total).

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Sep 6, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • rebutted — span=7bccecf1c5a7 (slot_buffers.py:437, function-local import lacks the circular-import comment)

Legitimate as a convention note, disproportional to fix now: the head is green after 11 review rounds, and a comment-only push re-arms every reviewer lane on the whole diff for zero behavior change. The import is function-local for the same chat_utils -> slot_buffers cycle as the identical function-local import a few lines up in the same file; the documenting comment will ride the next substantive touch of this file. This ruling covers convention-comment findings on this diff, wherever the line moves.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • rebutted — Watch: contract inversion without a compat story

The direction of the flip is the issue #4093 fix itself: the old "re-post after restart" guidance was damage control for a data-loss bug, not a stable contract — a restart could silently eat an acknowledged note, and callers had no way to know WHEN to re-post. The chosen failure direction is recorded in the PR body: an old-contract caller that still re-posts gets a bounded, VISIBLE duplicate; the old behavior was invisible loss. The App Kit reference and session spec were updated in the same commit as the migration signal.
The suggested caller-supplied idempotency id on POST /note is a good follow-up and the natural home for full old-client compatibility; it adds public API surface beyond this PR's approved scope (the maintainer scoped this PR to the durability invariant in the recorded ruling), so it belongs in its own reviewed change.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • rebutted — Watch: invariant load concentrated in prose

The load-bearing rules (union never shrinks, retirement is row-derived, drop records consumed only after the committed write, 200 only on positive evidence) are each pinned by dedicated regression tests in test/test_deferred_note_persistence.py (37 tests) — a future writer who violates one gets a red test naming the rule, not a silent regression; the comments are the explanation, not the enforcement.
The suggestion to give ConversationLog small public reads so slot_buffers stops touching _read_metadata/_path is fair layering cleanup; on an 11-round green head it is churn with no behavior change, so it should ride the next substantive change to the log's read surface.

…start (#4093)

POST /api/chat/slots/{slot}/note replies 200 with visibleDeferred: true for
a note held during a running turn, but both halves of the hold lived only
in _ChatSlot._deferred_notes — a gateway restart between the 200 and the
next turn silently voided the delivery promise.

The hold now persists through the slot's own metadata line under one
invariant: retirement is ROW-DERIVED. Every note carries an id; the flush
stamps each delivered inject row with it (meta.noteId) and records
rebind-dropped ids on the slot; the full save — reading the on-disk and
live holds UNDER the history lock — retires exactly the entries whose
rows are in the window it writes (or whose ids were dropped) and keeps
everything else. Row and retirement land in one atomic file replace; the
drop records are consumed only AFTER that write commits, since a dropped
note's row never exists and the record is its only retirement path.

- enqueue (durable-before-200): asyncio.to_thread + update_metadata_if,
  everything read inside the lock-time guard; the write MERGES by note id
  (union_deferred_notes), pins the posted note (ensure=) so a racing
  turn-end flush cannot yield a 200 with no durable copy, and pins the
  TARGET to the history key authorized at enqueue, re-verified under the
  store lock — a cron/workflow rebind in the persist window is refused
  (uniform not-found shape) instead of writing app content into a foreign
  transcript's metadata. The union never evicts: at the 2x ceiling the
  NEW note is refused (429). On EVERY failure branch, a rollback that
  finds the note already drained means a flush DELIVERED it — the 200
  stands, because any error would make the caller re-post a line the
  user already saw. Other failures roll back BY IDENTITY and answer a
  retryable 503 (including an UNREADABLE record)
- deferred notes are bounded at the ENQUEUE boundary (413 over 4000
  chars); the durable copy is persisted VERBATIM, never truncated
- both restore paths replay the hold up to the durable CEILING (2x the
  live cap — every durable entry is a 200-acknowledged note); restored
  notes are sanitized fail-closed (no session stamp -> dropped;
  over-bound content -> dropped; malformed context half dropped alone)
- the flush never writes metadata; a crash between flush and save
  re-delivers on restore (at-least-once)
- docs updated in the same commit (App Kit api-reference, session.md,
  endpoint docstring): do not re-post after a restart; 503 = retry,
  413/429 = boundary refusals, 404 = ownership/rebind refusal

_pending_context (the /context queue) stays memory-only: only the context
halves embedded in held notes ride the same metadata shape naturally.

Closes #4093
@NicholasRBowers
NicholasRBowers force-pushed the fix/persist-deferred-note-hold-4093 branch from dc1f8eb to 0749128 Compare September 7, 2026 00:12
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 7, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • needs-a-decision — Watch: 4,000-char mid-turn cap is a timing-dependent contract regression
    self-added: no

The cap protects the metadata line, which is rewritten WHOLE under the cross-process history lock on every mid-turn POST — the verbatim durable copy is what the 200 promises, so the bound is a real cost, not an arbitrary number. Both remedies the lane names are maintainer calls: raising the bound to the shared 40,000 changes the lock-hold cost profile, and moving the durable copy out of the metadata line is a storage redesign beyond this PR's intent.
Question for @NicholasRBowers: (a) keep 4,000 and add one API-reference sentence naming 4,000 as the effective safe bound for any caller that cannot observe turn state (smallest change, follow-up or in-PR at your call), (b) raise MAX_DEFERRED_NOTE_CHARS to 40,000 and accept larger locked rewrites, or (c) defer a storage split to its own issue. This same question was raised at the round-9 escalation; recommending (a).

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • rebutted — Watch: downgrade replay hole invisible to operators
    self-added: no

The version-skew caveat is documented where the invariant lives (docs/system-specs/modules/session.md names the downgrade-deliver-reupgrade duplicate replay and its bounded, at-least-once direction). A release-notes line is release-process work owned by the maintainer at ship time, not a change this diff can carry — this comment is that flag: when this ships, note that deferred-note retirement requires the delivering gateway to stamp meta.noteId, so mixed-version fleets can replay delivered notes as duplicates.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • rebutted — Suggestion: expose a public ConversationLog read/probe instead of private access
    self-added: no

Disproportional for this PR: slot_buffers.py is the dashboard package's own persistence-helper layer, and the restore/save machinery in the same package already reads _read_metadata/_path as a package-internal convention — these two probes add no new coupling class. A public probe API on ConversationLog would be an interface added for exactly two same-package callers; if the log's file layout ever needs to be sealed, that refactor should move ALL package-internal readers at once, not start here.

@iamwhatever
iamwhatever merged commit d9abfb8 into main Sep 7, 2026
71 of 73 checks passed
@iamwhatever
iamwhatever deleted the fix/persist-deferred-note-hold-4093 branch September 7, 2026 20:41
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 7, 2026
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.

Persist the deferred-note hold so a 200 is not voided by a gateway restart

3 participants