fix(dashboard): snapshot the slot view, gate the persist on identity, reconcile post-commit - #5933
fix(dashboard): snapshot the slot view, gate the persist on identity, reconcile post-commit#5933rnoack1 wants to merge 1 commit into
Conversation
|
👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review. When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically. |
|
👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review. When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically. |
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of I have enough grounding: the core mechanics (snapshot, commit-first ordering, identity-gated persist, the Design-Verdict: CONCERNS Real corruption bugs, correctly fixed — but most of the diff is self-declared interim scaffolding bridging to #8361, and the PR's own lock-rejection argument applies to it. Watch
Suggestions
[DESIGN-REVIEWED] bf4f60f |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All source hunks read, key claims verified against the base tree (the live-view awaited loops at First-Principles-Verdict: CONCERNS The Not justified as shipped
What this change shipsIntent: FIX — deleting a folder or tag while a session closes concurrently must not crash mid-sweep and leave conversations half-unfiled or tags half-stripped. (More than 10 items; the 10 most visible kept.)
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] bf4f60f |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
5fcb630 to
3040073
Compare
3040073 to
30431ab
Compare
30431ab to
30b2a43
Compare
9a29748 to
957ef40
Compare
957ef40 to
1b8720e
Compare
1b8720e to
37bb9c2
Compare
|
Problem: This branch still deletes against the pre-#5432 handlers. Main already landed folder tags on the same four files ( Fix: Rebase onto current main and put #5432’s three new paths on this PR’s vocabulary, don’t restore
Add the commit-first unfile and restore-time |
|
@kyleseaman re-verified each item at the current head
One item from your comment is not closed, and I am not claiming it is: the spec sentence you asked for about commit-first unfile and restore-time pruning belongs in a Chat Folders doc this PR does not touch (its only docs file is |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
Thanks — taking the one code-level ask in the audit, and leaving the ordering calls to you. The #7714 item is satisfied at head
The replacement assertion is One thing worth flagging, because it supports the audit's framing: #7714's own Not touched, deliberately. The census bound is still Separately, and not a request for action from you: the three red lanes on this head ( |
|
Coordination from the #7779 side, following the maintainer's 2026-09-04 relationship audit (it flagged a hard ordering dependency between these two PRs in Proposal: land #5933 first, and #7779 pays the adaptation. The No action needed from you unless you'd rather sequence it differently. |
|
Agreed on the sequencing — no objection to this landing first, and thanks for offering to carry the adaptation. I re-measured the two claims against my current head
So there is no file-level collision to resolve — which suggests the dependency the audit flagged is semantic rather than textual, and on that point your read of the direction is right. The persist is identity-gated and pinned to the pre-await transcript key, not re-resolved late: One caveat so it does not surprise you: the required-parameter shape is the part most likely to need a touch on your side, since any call into the sweep-persist path must now pass the key explicitly. That surface is also interim — it is slated to retire with the deferred merge-aware-save layer decision, tracked in the linked issue — so it is worth pinning against the issue rather than against this shape. |
|
@rnoack1 This is still the only place the live- Two open PRs add code inside the Three add a new src Smaller re-anchors: #7779 (@jeeshofone, already agreed to land after this one) in One rebase question: merged #8906 added Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
…oops Awaiting inside `for slot in state._slots.values()` raises "dictionary changed size during iteration" when another coroutine pops a slot, so iterate a snapshot. The snapshot holds objects rather than keys, so each loop also re-checks identity immediately before mutating and withholds the persist when the key is absent or REBOUND: a full save rebuilds the SLOT_OWNED_META_KEYS line where an absent field means cleared, so force-saving a pre-close object drops closed=True and resurrects a dismissed tab. Folder delete arms the withheld clears only once `mutate_folders` has committed, and re-scans afterwards for a conversation filed into the folder mid-flight, because the loader reads folder_id back without validating it against the folder list. Also adds an AST build gate that flags any live-view slot loop containing an await, with a `# loop-ok: <reason>` suppression that must state a reason.
Problem / Motivation
api_chat_folder_deleteandapi_chat_tag_deleteeach walk every slot and persist it. Since that persist becameawait save_slot_off_loop(...), each iteration now contains a yield point — but both still iterated the livestate._slots.values()view: the unfile loop insrc/kiro_crew/dashboard/chat_folders.pyand the tag-strip loop insrc/kiro_crew/dashboard/chat_tags.py, each with theawaitnested directly inside.state._slotsis mutated by other coroutines that can run during that yield —session_transfer.pyandsession_control.pypop keys,openai_compat.pypops on its cleanup paths, andget_or_create_slotassigns. So deleting a folder or a tag while any session closes concurrently raisesRuntimeError: dictionary changed size during iterationout of the handler.Before the persist was awaited, these loop bodies had no yield point and the iteration was effectively atomic against other coroutines, so the live view was safe. Adding the
awaitis what made it reachable.Why it matters
The raise is worse than a 500, because both loops mutated before they persisted, so it landed with the work half-applied:
folder_idcleared, others not.Both leave durable state inconsistent, and the trigger is ordinary concurrent use rather than anything exotic: one tab closing while the user tidies folders or tags in another.
Chasing that down surfaced three further ordering defects on the same two handlers, each reachable by ordinary concurrent use and each producing a durable wrong value rather than a crash. They are fixed here too, and described below in the order they were found.
What changed (motivation → approach → change)
Everything this change ships, in one list (each item is expanded below):
list(state._slots.values()), so theawaited persist cannot mutate the iteration.
live
_slotsentry, so a close that committed inside the window is not erased.without the delete that justified it.
window is still reached.
SLOT_OWNED_META_KEYSentry and erases a concurrent close'sclosedflag.persist_swept_slot_meta, not duplicated at eachcall site, so a future sweep site cannot omit it.
_committed_folder_ids/_committed_tag_ids)published only after a confirmed write, with
None= UNKNOWN (fail open) andfrozenset()= KNOWN-EMPTY (prune) kept distinct.One reader for the tag prune —
state.tag_ids_for_restore, replacing fourhand-synced copies of the fail-open rule;
_tags_authoritativeis retired entirely.Publication routed through
_commit_tags_snapshot, unconditionally after aconfirmed write, and gated on the tags file actually existing.
Eight AST gate families — no awaited loop over a live
_slotsview, and no tag-snapshotwrite off the
_commit_tags_snapshotchain.Cancellation atomicity — a new module,
snapshot_commit.py, carryingsweep_to_completion_despite_cancellationand a publish-exactly-once commit helper, pluscapture-and-re-raise choreography in BOTH delete handlers: a client disconnect mid-delete
still finishes the unfile sweep and still emits the operation's only audit line, and the
cancellation is re-raised once, after every durable consequence.
Parked slots revalidate before re-entering the registry — the two app-teardown
restores in
spec_builder/backend/runtime.pyre-registered a whole slot object intostate._slots, which no attribute-write gate can see, so a slot parked BEFORE thesweep's capture was reached by neither pass. Folder and tag ids are now revalidated
against committed membership read before the await.
Channel default filing refuses a folder that died mid-window — the default-filing
branch of
surface_channel_sessionno longer files a slot into a folder a committeddelete removed while the arrival was in flight.
A commit-window rebind cannot move a sweep's write off the record that carries the id
— routing can change before pass one reads it, so each sweep retains every matching slot
together with the transcript key it held BEFORE the commit, and pass two pins that key as
expected_history_key(required, not defaulted). Without the pin the write lands onwhichever transcript routing moved to, leaving the deleted id durable on the original.
The COLD-START paths are deliberately NOT touched —
_rehydrate_slot_from_history,_apply_recent_session,api_chat_slot_resumeand themeta["folder_id"]branch ofsurface_channel_sessionadopt the persistedfolder_idVERBATIM, exactly as the base does.An earlier revision of this PR routed all four through
state.folder_id_for_restorefor ashape-only check; that rider was REMOVED, because its only effect was rejecting a malformed
stored id the base kept harmlessly — the sidebar renders any unknown id in the Unfiled bucket
(
website/src/utils/groupHistoryByFolder.ts:40), so the crash it prevented was one thevalidator itself introduced. The validator's remaining call sites are all race closures: they
prune only on a committed-present → committed-absent transition a caller observed itself, plus
the delete sweep's own superseded-merge adoption.
Residual hazard, stated here and not only in the spec. A readable-but-stale
folders.json— a restored backup, a half-synced data home — parses as KNOWN, so theNone=UNKNOWN fail-open rule cannot catch it. Because the cold-start paths now validatenothing, that hazard cannot reach them: a stale store can no longer unfile a filing made after
its snapshot. One sibling remains declared rather than closed: the DEFAULT-FILING branch of
surface_channel_session, which has no transition evidence to withhold on. The tagvocabulary is not one of them. Its cold-start readers adopt persisted tags verbatim,
and every mid-session site holding a pre-await observation threads it as
committed_before, so a tag is pruned only on a committed-present -> committed-absenttransition that site watched happen.
api_chat_slot_resumekeeps the plain membershiptest on purpose: its KNOWN-EMPTY arm is what stops a crash mid-delete resurrecting a
dangling id forever. No durable on-disk evidence file backs any of this, deliberately:
the dangling
folder_idsuch a file would purge is one the tree already absorbs (the sidebarbuckets it as Unfiled and resume clears it under the lock-held existence verdict), and the file
would add a second forgeable input in the same directory as the vocabulary. A prune licence
comes only
from a transition an in-process caller observed itself. See
history.md.chat_utils.run_config_writenow awaits the sharedsnapshot_commit.drain_shieldedinstead of carrying its own copy of the shield-drain-reraise loop — subtractive, deleting a third spelling of one protocol. The caller still derives its own outcome, because a config write publishes nothing.Observed symptom:
dictionary changed size during iterationfrom two delete handlers under concurrent session churn. Root cause: a yield point was introduced inside an iteration over a live dict view whose backing dict other coroutines mutate. The change addresses that cause directly — iterate a snapshot, so the loop's view cannot be invalidated by a concurrent pop:This is the form the slot-wide loops in
state.pyalready use, so it is the codebase's existing convention rather than a new pattern.Second — concurrent-close arbitration, and it does alter the success path. The snapshot holds slot objects, not keys, so it stops the crash but not a durability bug behind it: across the
await, another task can CLOSE a slot, which pops it fromstate._slotsand persistsclosed=True. Force-saving the pre-close object then writes it back over that close, and becauseclosedis slot-owned metadata where an absent field means "cleared", the close is erased and the dismissed tab returns on the next restore. Each sweep therefore re-checks identity before persisting, and withholds the persist when the key is absent or rebound:israther thanis not Nonebecause the key can be REBOUND to a different object (get_or_create_slotassigns), and mutating the replacement is the same bug wearing a live key. This is a behaviour change on the success path: a sweep can now skip a persist it would previously have performed, which is the intended effect. Two sites carry the gate — the folder delete's post-commit sweep and the tag delete's strip sweep.The gate is deliberately on the PERSIST only, never on the in-memory clear. The two have opposite requirements and one guard cannot serve both: an in-memory clear writes nothing, so it can never erase a persisted
closed=True— only the force-save can — while skipping the clear as well would leave a slot that was merely transiently absent (popped by an in-flight close whose save then fails, so the close handler restores it) still naming the deleted folder or tag.Third — the folder removal commits FIRST. The unfile used to run before
mutate_folders, which meant a folder write that failed had already mutated slot objects a concurrent close might be serialising: the close persisted the clearedfolder_id, and the rollback was withheld on the same identity test that withheld the original persist, so nothing repaired it. Folder still present, conversation durably Unfiled. Committing first means a failed write has mutated nothing at all, which is also the orderapi_chat_tag_deletealready used. The unfile is now the post-commit sweep, which covers a slot filed into the folder mid-flight as well. This removes the pre-commit unfile loop, thewithhelddeferral list and_restore_unfiled— with the commit first there is nothing to defer and nothing to roll back.Fourth — the sweep is ONE round of two passes.
One round, no bound. An earlier iteration of this change repeated the sweep up to a fixed bound, and it is worth recording why that is gone rather than silently dropping it. One residual producer could hand a deleted id to a slot the sweep had already passed: DEFAULT FILING, the fourth copy site in
surface_channel_session. That branch does validate, on the same shared helper — the gap was never a missing check. It was that the helper had to fail OPEN while the folder-store lock was held, because a locked store may be showing a removal that is about to be rolled back, so an arrival inside that window kept the deleted id:That value is the channel's configured folder, handed in by
reconcile_channel_slots, which resolves it across one await (lookup_channel_folder, read-only and under the store lock, so it sees committed state) and persists it across another before surfacing. A delete committing inside that window cannot be seen by a value that was already resolved and already correct when it was read, so no check at the source can help — and the slot is created byget_or_create_slotAFTER this handler took its snapshot, so the snapshot cannot see it either.That window no longer exists, which is what retired the repeat.
folder_id_for_restorevalidates against a COMMITTED vocabulary snapshot: a frozenset of folder ids thatmutate_foldersswaps in only AFTER its write confirms, and never on the rollback path. So the helper cannot observe uncommitted state and needs no lock probe to protect itself from it — the probe, and the store-wide fail-open it caused, are both gone. Because this handler's own commit runs first, the folder is out of the committed vocabulary before any arrival can be assigned it, and a default-filed arrival is therefore pruned at assignment rather than chased afterwards. With no residual producer left, one round is enough: there is no bound, and nothing for a second round to find.Two passes. The persist is an
await, so anything that COPIES slot metadata while it runs — a fork — reads whatever a single interleaved loop has not reached yet, and writes that stale value into a NEW record outside the snapshot. Pass one therefore clears every matching slot and contains no yield point at all, so no copier can observe a half-swept set; pass two does all the awaiting, by which point every slot in the set already reads cleared. That yield-free property is the fork-copy guarantee, and it is why the split survives the collapse to one round: it was never a termination proof. With a single round there is nothing to terminate, and pass two over an empty set is already a no-op.No third pass, in either delete handler. An earlier revision of this change swept the live view once more after pass two, to catch a writer that put the deleted id back on a live slot — or onto a slot that did not exist when pass one ran — inside pass two's await window. That sweep is NOT in this diff. The only producer that could reach it was
api_chat_slot_fork, and this change validates the fork at its source instead, so the sweep had no remaining producer to catch and both handlers drop it.docs/system-specs/modules/history.mdis the durable statement of the protocol, and it records what a NEW writer must do in the sweep's absence: validate at its own source, or keep its vocabulary check and its assignment in one synchronous run.Which producers exist, measured rather than assumed. The one producer a downstream sweep would have had to chase is
api_chat_slot_fork: it registers its child instate._slotsand then copies the parent'sfolder_idandtags, and at the base it did so through neither validator nor any read of the committed vocabulary, so a delete landing in its await window made a dangling id durable on a record no snapshot ever saw. This change routes both copies throughfolder_id_for_restoreandtag_ids_for_restore, which removes the producer instead of sweeping after it. The other demonstrated producers were the two REFUSED-MOVE reverts, which capturepreviousbefore anawaitand restore it verbatim when the target turns out to be gone:api_chat_slot_folderandapi_chat_slot_create; both are fixed at source the same way. Every remaining writer ofslot.folder_idorslot.tagsis safe for a stated structural reason rather than by being swept — it holds the delete handler's own write lock, or its vocabulary read and its assignment have noawaitbetween them, which coversapi_chat_slot_folder's target check andcreate_session's lock-held folder check alike.Fifth — the matching slots are captured before the commit. Commit-first put the sweep's snapshot after the folder write, and a concurrent close pops its slot for the whole of that write, so such a slot is in no snapshot the sweep can take. The bounded repeat does not rescue it either: the first pass finds nothing left to clear and stops. When the close's own save then fails, the handler puts that same object back into
state._slotsstill naming a folder that has just been deleted. So the handler now holds the matching slot objects across the commit:Capturing is safe where clearing was not, and that distinction is the whole reason this does not reopen the third defect: a capture only READS, so a failed folder write still mutates nothing. The clear pass then walks the captured objects and the live view together. A slot in both is visited twice and that is harmless — the first visit clears
folder_id, so the second takes thecontinue.Also adds eight AST gate families, because these defects are invisible at the call site. Two of the eight carry a maintained list and are the whole of the permanent contributor cost: the 13-entry
_UNVALIDATED_VOCABULARY_WRITERSadopter allowlist, and the_FORCE_SAVE_CLOBBER_SITEScensus (an exact pin: removing a site reddens the build too, so the count is updated with it — deliberately, because the pin is the forcing function for removal).docs/system-specs/modules/history.mdenumerates all eight and states that cost. The first: nothing aboutfor slot in state._slots.values()looks wrong until you notice theawaitnested inside it, and the next slot-wide loop someone writes will read just as naturally. It flags anyfor ... in <x>._slots.values()whose body contains anawaitin the same frame, and covers.values(),.items()and.keys()alike — the hazard is a property of the lazy view, not of which projection the loop happens to read, so a gate covering only the spellings already in the tree would stop the defect that was written and not the one that will be. It matches the attribute chain rather than a receiver name, sostate,selfanddsare all covered. It ships with NO suppression mechanism: there are no sanctioned awaiting live-view loops insrc/, so an escape hatch with zero users would be dead surface that only weakens the gate.The second gate pins the tag-snapshot write chain. Every prune path trusts
_committed_tag_idsabsolutely, and what keeps that set truthful is publication immediately after a confirmed write inside_commit_tags_snapshot. Folders get this structurally because every folder write routes throughmutate_folders; the tag side does not, so the gate asserts that nothing undersrc/reachessave_tags_snapshotor_write_tags_snapshotoff the sanctioned_commit_tags_snapshot→_write_tags_snapshot→save_tags_snapshotchain. It matches bare name references as well as calls, because the sanctioned hop isasyncio.to_thread(_write_tags_snapshot, ...)and a call-only detector would miss both that and any bypass spelled the same way.Bounds on the claim
awaitcannot. The persist is no longer a full slot save: these two sites now use a metadata-only merge (persist_swept_slot_meta, which delegates the merge step to_merge_slot_meta) that writes justfolder_id/tagsunder the record's own cross-process lock, so a close committing during the persist is left standing instead of being rebuilt away. The merge's existence check is re-taken INSIDE that lock viaupdate_metadata_if, with a caller guard confirming the record still names the folder / carries the tag being deleted — so a session deleted in the window is not upserted back, and a reassignment that already reached disk is not overwritten. The otherforce=Truecall sites in the repo are unchanged.state._slotsfor good, so arming_dirtyon it is inert and its persisted line keeps the dangling id until something else saves it. Read-side validation offolder_id— mirroring the tag vocabulary prune the loader already performs — is the right root fix, and this PR now ships it. Of the four sites that copy afolder_idonto a slot, ONE prunes it against the folder vocabulary — the default-filing branch ofsurface_channel_sessioninchannel_slots.py. The other three adopt the persisted value VERBATIM, as item 12 states: both restore paths inchat_persistence.pyand themeta["folder_id"]branch ofsurface_channel_session. An earlier revision routed all four through the validator; that rider was removed, and this sentence had not been updated with it. Four further readers route through the same helper — the superseded-merge adoption inapi_chat_folder_delete, the History-resume path inchat_handlers.py, and the refused-move reverts in BOTHapi_chat_slot_folderandapi_chat_slot_create— twelve call sites in total once the fork producer and the reverts are counted. The two reverts are siblings and are fixed together here: the create path performs no target pre-check at all, relying solely on_unhide_folder's lock-held verdict, so its revert is the only place its stale capture can be caught. Each fails OPEN when the vocabulary is unknown (folders.jsonunreadable), since pruning then would unfile every conversation; a legitimately-empty vocabulary is authoritative and does prune. That unknown-vocabulary case is the ONLY fail-open that survives: the helper validates against a committed vocabulary snapshot, published only after a folder write confirms, so it has no uncommitted state to guard against and the unfile sweep is a single round with no residual producer to chase.Accepted and deferred, with the layer-level option named so the next reader inherits the decision. Three seams are deliberately not in this PR. (1) The folder side's
_adopt_observed_placementand the tag side's_adopt_observed_tagsboth validate their observed value: the tag one againstpre_delete_committed, the committed vocabulary captured at transaction start, under the same None-fails-open rule. (2) Tensave_slot_off_loop(..., force=True)sites remain unconverted, so the full-save-erases-a-concurrent-close hazard is closed at the two sweep sites and not at the others. That count is AST-measured: nine are indashboard/and a tenth is increw_chat.py; three of the ten span multiple lines, which a single-linesave_slot_off_loop(.*force=Truegrep misses. All ten are annotated in place, and the count is pinned by a gate so the deferral cannot grow unnoticed. That hazard is a property of the SAVE FORMAT —SLOT_OWNED_META_KEYSis rebuilt wholesale and an absentclosedreads as cleared — and converting sites one at a time costs a bespoke guard/adopt closure each, which scales linearly in complexity. So before repeating this at the remaining ten, the follow-up should weigh fixing the layer once instead: either a merge-aware save, or persistingclosedPOSITIVELY so absence is no longer clearing. That is the decision to take first, not after the seventh conversion. (3) A third repo gate could pin the metadata-only-merge invariant structurally — a function-scoped AST walk asserting neither delete handler contains asave_slot_off_loop(..., force=True)call — which is implementable but would add machinery to answer a concern about machinery, and two behavioural tests already cover that invariant. The sweep-merge protocol no longer relies on prose to be read end to end: the three-way outcome is a namedSweepMergeOutcomeat the callback boundary, the disposition dispatch inpersist_swept_slot_metais exhaustive over it -- its final arm assigns to aNoReturn-annotated name, which only type-checks once every member is handled -- so a member without a disposition fails type-checking on every supported interpreter, and a guard returning a non-bool is rejected at the boundary rather than truthy-tested. Three tests pin that protocol independently of the two existing sweep sites.surface_channel_sessionarm_dirtyafter the window rebuild so the flush rewrites the record, and neither arms when the placement stands.folder_idrestore paths outside these two handlers now route through the same validator. The History-resume path additionally keeps its own_unhide_folderprune, which acts on an existence verdict taken inside the folder-store lock and scoped to the id that verdict was computed for; the shared helper is an unlocked reader and cannot replace it, so the two compose.Tests
test/test_slots_snapshot_across_await.py(89 tests):test_no_slot_wide_loop_awaits_over_a_live_viewfails the build on any live-view slot loop containing anawait, plustest_the_gate_scanned_a_non_empty_treeas a positive control so an empty scan cannot let the gate pass vacuously.test_folder_delete_survives_a_concurrent_slot_popandtest_tag_delete_survives_a_concurrent_slot_popdrive the actual aiohttp routes with asave_slot_off_loopstand-in that pops a different slot during the awaited save. That models the real concurrent popper deterministically rather than racing for it. Each asserts the pop genuinely fired before asserting the outcome — otherwise the test could pass by never opening the hazard window — and then asserts the surviving slots were fully unfiled / stripped.test_every_slot_wide_mutating_loop_carries_the_identity_recheckas a structural check that no slot-wide mutating loop is missing the re-check.test_failed_folder_delete_does_not_durably_unfile_a_closing_slotfails the folder store write after a modelled close has popped and serialised the slot, and asserts the conversation is not left recorded as Unfiled while its folder survives.test_folder_delete_sweeps_every_slot_before_the_first_save_awaitsand its tag-side twin have a fork copy an unswept slot's metadata during the first save's await and assert the copy reads cleared;test_folder_delete_resweeps_for_a_default_filed_slot_surfaced_during_persistencedefault-files a channel slot into the folder during the persist pass, through the real surfacing helper, and asserts the repeat catches it — with a path control proving the arrival took the default-filing branch, and its siblingtest_folder_delete_prunes_a_metadata_surfaced_slot_at_the_sourcecovering the branch the prune does close;test_slot_published_after_the_snapshot_is_still_unfiledcovers the mid-flight file.test_folder_delete_clears_a_slot_popped_during_the_folder_writepops the slot duringmutate_folders, lets the removal commit, then models the close's save failing and restoring that same object, and asserts it comes back unfiled with the flush armed.statereceiver; stays quiet on the snapshot shape, on a live view with noawait, and on anawaitinside a nested scope.Every behavioural test above carries at least one negative control that also holds on unfixed code — that the pop fired, that the fork ran inside the await window and wrote a record it could copy from, that the removal committed — so a vacuous green is detectable rather than silent.
Observed on this revision:
test_slots_snapshot_across_await.py+test_chat_tags.py→ 103 passed, 0 failed; the folder / rate-limit set (21 files,test_*folder*.py+test_*rate_limit*.py) → 648 passed, 0 failed;test_persist_off_loop.pyplus the folder ownership / app-isolation / audit-origin suites andtest_channel_slots.py→ 130 passed, 0 failed.flake8andmypy(both at the versions pinned inpyproject.toml) are clean on every changed file, and the black formatting gate reports no new offender.Manual verification
N/A — unit coverage sufficient. The original failure is a deterministic
RuntimeErroron a specific interleaving, and each of the four ordering defects is likewise a specific interleaving; the behavioural tests reproduce those interleavings exactly against the real routes, which is stronger than a manual attempt to hit the races by hand.Related Issues
close. This change defers that layer fix and bridges it with
persist_swept_slot_meta, the_FORCE_SAVE_CLOBBER_SITEScensus gate (10) and the_UNVALIDATED_VOCABULARY_WRITERSadopterallowlist (13). All three are interim and retire together when that issue lands; each now points
at it in-source, so the retirement plan is anchored outside a doc paragraph.
Pattern harvest
Rule candidate: review-checklist (argued below against a static rule, with counts)
Pattern: a handler reads a value out of shared state,
awaits, and then writes thatvalue back or acts on it without re-checking that the world still supports it. The
suspension is the whole defect: whatever made the value correct when it was read can
be undone by another writer while the coroutine is parked, and nothing on the resume
path re-asks.
Why it generalises — recurrence, not a single instance. The identical shape
occurred TWICE in this change, in two different handlers, as a refused-move revert:
api_chat_slot_folderandapi_chat_slot_createeach capture the slot's currentfolder_idbeforeawait _unhide_folder(...)and restore it verbatim when thatawait reports the target gone. Both then leave the slot naming a folder a concurrent
delete removed inside the window. What makes this recurrence rather than one bug
counted twice: only the first was reported. The second was found by enumerating every
writer of
slot.folder_idafter the first was cited — so a fix scoped to the reportedsite would have shipped with its twin live. The same family also motivates the rest of
this change: the original crash was iterating a live
_slotsview across anawait,the identity re-check had to move inside
persist_swept_slot_metabecause a checktaken before the await cannot speak for the write after it, and
_adopt_observed_placementhas to treat itsobservedvalue as pre-await evidencerather than truth.
Outside this PR, one further instance of the same family is present in the tree at
src/kiro_crew/apps/builtins/code_review_sage/sage_lib/review_pool.py:362(
_ensure_runtime_locked,self._runtime = rt), measured both on this branch and onupstream main, so it predates this change and is not something this PR introduced.
Should it become a static rule? Measured no — and the measurement is the argument.
I wrote an AST detector and ran it over all 1233 parsed files under
src/kiro_crew/,in two strengths:
awaitand read after it — fires in688 functions across 1053 read-sites. That is ordinary correct async code, so as
a gate it is unusable; it would be turned off within a day.
await, assign that same localback onto an attribute — fires once tree-wide, and the one hit is the
review_pool.pyinstance above. It does not find either motivating instance,including on upstream main where both are unfixed. The reason is structural, not a
tuning problem: both reverts sit inside
if not await _unhide_folder(...):, so theyare nested one block below the statement level a simple walker inspects.
A deeper walker could reach nested blocks, but that only relocates the difficulty. The
predicate that separates a defect from correct code here is whether the captured
value's VALIDITY can be revoked by another writer during the suspension — which is a
question about the domain (is this id a key into a store another handler can delete
from?), not about syntax. A rule keyed on syntax alone would flag the 688 and still
miss the two that mattered.
So the durable artifact is a review question rather than a lint: when a handler
restores or re-uses a value it captured before an
await, ask what invalidates thatvalue during the suspension, and re-validate on the resume path rather than trusting
the capture. In this codebase the re-validation already exists as
state.folder_id_for_restore/state.tag_ids_for_restore, which is why both fixesare one call rather than new machinery. The checkable half is mechanical and cheap:
when a reviewer cites one such site, enumerate every writer of that field before
fixing, because this change is evidence that these arrive in pairs.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Alternative weighed: pessimistic mutual exclusion
One serialising lock over vocabulary deletes versus slot close / fork / surface was not
weighed above, and it deserves to be, because it collapses most of what this change builds.
What it collapses. If no close, fork or surface can interleave with a delete, then the
pre-commit key capture, the two-pass yield-free sweep, the post-await identity re-check, the
placement counter's ABA detection all become unnecessary:
each exists only because a slot can rebind or be popped inside the delete's await window.
That is the large majority of this change's machinery.
What it costs. The lock has to span commit and sweep, not just the commit — holding it
for the commit alone leaves the gap between the halves, which is the window that produced the
original defect. So its hold time scales with the number of slots filed into the folder, and
it is held across a disk write. The operations it blocks are the interactive ones — closing a
tab, forking a conversation, a channel message arriving — while the operation it protects is
rare. That inverts the usual trade: it makes a rare operation cheap to reason about by making
frequent, user-visible operations occasionally wait on a multi-write critical section.
Why the optimistic protocol was still chosen here. Not because the lock is wrong — it is a
coherent design and would be simpler to verify. It is because the lock and the deferred layer
fix solve the same problem, and the layer fix dominates: a merge-aware save (or positive
closedpersistence) removes the root cause — a full save rebuilding every owned metadata keyand erasing a concurrent close — without making any interactive path wait. A lock introduced
now would be superseded by that work rather than composed with it, so it would be a second
temporary structure rather than a replacement for this one.
The honest residual. If the layer fix is rejected or deferred indefinitely, the lock, not
this protocol, is the better permanent shape, and the comparison above should be re-run at that
point rather than treated as settled by this change.
Subtracted this round: the rebind-window transcript scrub
An earlier revision carried a SECOND key-addressed write on top of that pin: having found the
orphaned key, it re-visited that record to blank the vocabulary id, with its own guard shape because
the tag side derives its surviving list rather than writing a constant. The pre-commit key capture
itself is NOT what was removed — it is live in both sweeps and is what aims the write they already
make.
It is removed. The state it purged is the one this change's own ruling already accepts — a
persisted id naming no live folder, absorbed in four places, the third of which names this record
exactly ("a dangling
folder_idleft on the old transcript is ignored on the next load"). The onebehavioural consequence of a dangling id, a withheld auto-file suggestion from
maybe_suggest_folder, cannot arise for an orphaned record at all, because no slot routes to itfor that code to read. Keeping the scrub meant a durable write and a second guard shape (the tag
side needed a stricter one, since its write is derived rather than a constant blank) for a state
the tree does not treat as damage.
Also declared, previously omitted from the enumeration above:
chat_utils.run_config_writenowawaits the shared
snapshot_commit.drain_shieldedinstead of its own copy of theshield-drain-reraise loop. That deletes a third spelling of one protocol; the caller still derives
its own outcome, because a config write publishes nothing.
NEXT STEP, tracked rather than deferred. The merge-aware-save layer decision is #8361, and it is the tracked successor to this change rather than an open-ended maybe: the guard/adopt surface, the adopter allowlist, the force-save census and the gate families all retire with it. Two standing constraints hold until it lands — convert the remaining
force=Trueslot-metadata sites ONLY via that layer fix, never by repeating the bespoke guard/adopt pattern per site; and treat a red force-save census as the prompt to take the decision rather than to raise the pin.