[fix] A deleted young session no longer comes back on the next reconcile - #5830
[fix] A deleted young session no longer comes back on the next reconcile#5830moataz-hjaiji wants to merge 3 commits into
Conversation
The rail's delete only propagated to the server for a `serverKnown` session, but that flag lags the durable row: the row exists from the first message, while `serverKnown` flips only on the next successful reconcile. Deleting inside that window deleted locally ONLY, so the very next reconcile re-adopted the still-listed row as a brand-new server session — auto-titled, full content. The delete visibly undid itself. Fire the remote delete for any session, not just a `serverKnown` one, and record the id in a per-scope tombstone set. The reconciler refuses to adopt a tombstoned id and re-fires its delete until the server stops listing it, which also covers the two windows an unconditional request alone leaves open: a delete that failed (offline/5xx), and a server list fetched before the delete landed. Tombstones prune against the server list, so an id the server never had clears on the next reconcile and the set cannot grow without bound. Fixes Agenta-AI#5543
|
@moataz-hjaiji is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughSession deletion now persists per-scope tombstones, sends remote deletion requests for all sessions, retries failed deletions during reconciliation, and prevents tombstoned sessions from being re-adopted. ChangesSession deletion lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SessionStore
participant RemoteDeleteAPI
participant SessionServer
participant Reconciler
SessionStore->>SessionStore: record deletion tombstone
SessionStore->>RemoteDeleteAPI: request session deletion
RemoteDeleteAPI->>SessionServer: delete session
Reconciler->>SessionServer: list sessions
SessionServer-->>Reconciler: return server sessions
Reconciler->>SessionStore: retry retained tombstoned deletions
Reconciler->>SessionStore: skip tombstoned sessions
Reconciler->>SessionStore: adopt unrelated sessions
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes a session-rail reconciliation bug where deleting a recently-created session could be undone on the next server reconcile because the delete was previously gated on a lagging serverKnown flag.
Changes:
- Add a persisted per-scope “tombstone” set (
deletedIdsByAppAtom) to prevent re-adoption of recently deleted sessions and to drive retry deletes during reconcile. - Fire
deleteSessionRemotefor deletes regardless ofserverKnown, and retry deletes from the reconciler until the server no longer lists the session. - Add a dedicated vitest suite covering the regression and retry/tombstone behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
web/oss/src/components/AgentChatSlice/state/sessions.ts |
Adds tombstone persistence + reconcile behavior to prevent deleted sessions being re-adopted and to retry remote deletes. |
web/oss/src/components/AgentChatSlice/state/sessions.delete.test.ts |
Adds regression + durability tests for delete behavior using real atoms/store with the network boundary mocked. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses review feedback: the `...(args as [])` spread made the mock look zero-arity and skipped type checking of the call payload.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
web/oss/src/components/AgentChatSlice/state/sessions.ts:117
- Doc comment uses "Ids"; project style elsewhere typically uses the acronym "IDs", and the current phrasing reads a bit awkwardly. Consider updating to "IDs of sessions…" for clarity.
* Ids the user deleted whose server row may still be listed — a tombstone set, per scope.
web/oss/src/components/AgentChatSlice/state/sessions.delete.test.ts:54
- These tests exercise atoms backed by atomWithStorage with
getOnInit: true, so they can rehydrate prior localStorage state. Because the scopes are fixed strings (e.g. "delete-retry"), reruns in watch mode can become order-dependent/flaky if prior runs left data behind. Clearing the relevant localStorage keys inbeforeEach(or generating unique scope keys) will make the suite deterministic.
beforeEach(() => {
deleteSessionRemote.mockClear()
})
|
Reviewed this against the code — the fix is right, and I want to flag two things for whoever merges it. 1. This also closes #5831, not just #5543. The PR frames the bug as the window between "durable row exists" and "next reconcile stamps // state/projectSessions.ts
const isQueryableScope = (appId: string): boolean => Boolean(appId) && isValidUUID(appId)So for Note this also means the tombstone set in those scopes is never pruned (pruning happens in the reconciler, which never runs there). It is bounded by how many sessions a user deletes in a drawer, so not a blocker — but it never self-clears there, unlike in an app scope. 2. Agreed with the scope call in the PR body — the symptom is different enough (the optimistic One detail I want to explicitly endorse: |
Review feedback: the non-UUID scopes (__global__, drawer:<id>, onboarding) never run a reconcile, so serverKnown was never set there and the delete stayed local permanently rather than for one poll cycle (Agenta-AI#5831). Those scopes also never prune tombstones — harmless, since nothing re-adopts there, but the comment claimed pruning bounds the set unconditionally.
|
Thanks @ardaerzin — this is a better characterisation than mine. Verified point 1 against the code and you are right: Took the tombstone-pruning note too — my comment claimed pruning bounds the set unconditionally, which only holds in a reconciling scope. Corrected in e4dd360: in the non-queryable scopes nothing prunes, but nothing can re-adopt there either, so the tombstone has nothing to guard and is bounded by how many sessions the user deletes in that scope. I left the behaviour alone rather than special-casing the scope check into this atom, since you called it a non-blocker — say the word if you would rather it skipped tombstoning entirely when the scope is not queryable. On point 2: opened #5861 for the archive/unarchive gate so it does not get lost. Happy to reframe or close it if you had a different shape in mind. |
Summary
Fixes #5543 — a session deleted shortly after creation reappears, auto-titled and with its full content, on the next reconcile.
Fixes #5831 — deleting from Session History sends no backend request at all, so the session survives on other clients. Same gate, permanent rather than transient; see the scope note below (thanks @ardaerzin for spotting that these are the same root cause).
Root cause. The rail's delete only propagated to the server for a session the client had marked
serverKnown:But that flag lags the durable row. The row is created as soon as the first user message exists —
autoTitleSessionAtomFamilycallssetSessionHeaderfrom an effect inAgentConversation.tsx— whileserverKnownonly flips on the next successful reconcile (staleTime: 30s,refetchInterval: 60s). Delete inside that window and the delete stayed local only: the server row survived, and because the id was no longer in the local list, the very next reconcile treated it as a session it had never seen and re-adopted it. That is also why the resurrected session comes back auto-titled — the server had the header all along.This explains the report's "~69s" observation: the window is not a fixed 60s, it is "until the next reconcile actually runs".
And in some scopes it never runs at all.
projectSessionsQueryAtomFamilyis gated onisQueryableScope, which requires the scope key to be a real app UUID. The__global__,drawer:<entityId>andonboardingscope keys are not UUIDs, so the query is disabled,reconcileServerSessionsAtomFamilynever fires, andserverKnownis never set on anything in those scopes. There the delete was local-only permanently — no DELETE request ever sent, session alive on every other client. That is #5831, and it is the same gate rather than a separate bug.Fix. Two parts, because firing the request unconditionally is not sufficient on its own:
deleteSessionRemotefor any session, not just aserverKnownone. This is safe:callFernswallows non-abort errors and returnsnull, so a 404 for a session the server never had logs and moves on — no unhandled rejection.deletedIdsByAppAtom, persisted alongside the other session atoms). The reconciler refuses to adopt a tombstoned id, and re-fires its delete until the server stops listing it. This closes the two windows an unconditional request alone leaves open:Two details worth flagging for review:
if (!changed) returnearly-return in the reconciler. The steady state after a failed delete is "server list unchanged", which would otherwise skip the retry forever.I also added one guard beyond the report: an id that is back in local history (a deep link re-adopts by id) drops its tombstone. Without it, a stale tombstone would keep re-deleting a session the user deliberately reopened.
Scope note:
archiveSessionAtomFamily/unarchiveSessionAtomFamilycarry the sameserverKnownguard and have a related-but-distinct symptom (the optimisticarchivedflag is reverted by the next reconcile rather than the session being resurrected). I left them alone to keep this PR to the reported bug — happy to open a separate issue if you'd like.Testing
Verified locally
Dev stack via
./hosting/docker-compose/run.sh --oss --dev --web-local, comparingmainagainst this branch.main: create a session, send one message, delete it from the rail within ~30s, refresh — the session returns, auto-titled, with its content.Added or updated tests
web/oss/src/components/AgentChatSlice/state/sessions.delete.test.ts— 5 tests using the real store, real atoms, and the real reconciler, with only the network boundary (@agenta/entities/session) stubbed:I checked these are not vacuous:
expected [ 'young-2' ] to deeply equal []);!existingIds.has(id)guard makes it fail.Full slice suite green:
19 test files, 137 tests passed.prettier,eslint, andtsc --noEmitclean on both files.QA follow-up
drawer:<entityId>scope).Demo
https://www.loom.com/share/7e3497a1086c4ae2b399c1f377f09903
The recording compares
mainagainst this branch: create a session, send one message, delete it from the session-history popover within the pre-reconcile window, then refresh. Onmainthe session returns, auto-titled; on this branch it stays gone.Checklist