Repository navigation
fix(core): revert admitted session input - #62727
AdityaxDeore wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
github-actions
Bot
added
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core part of the fix works: staging and committing a revert to an admitted-but-unpromoted prompt now succeeds, where on the base both fail with MessageNotFoundError. I checked this by running the new revert tests against the base (they fail there) and with this PR (they pass). The revert, prompt and projector tests pass (106), and packages/core type-checks.
Public API: unchanged. The only visible difference is that session.revert.stage/commit now accept an admitted inbox ID where they used to return "message not found".
One change I'd ask for before merge: the commit projection for an already-promoted boundary now finds its inbox cutoff by reading the raw event log. That only works when events are persisted, which is off by default for the local server. See the inline comments. #39736 only needs the unpromoted path, so I'd drop this part and keep the previous message.seq cutoff for promoted boundaries.
Related: #57383
| // before its delivery still belongs to the reverted suffix. Without | ||
| // an enqueue record (e.g. a forked message) fall back to the | ||
| // message seq, the previous behavior. | ||
| const admitted = yield* db |
There was a problem hiding this comment.
This makes the projection depend on the raw event table, which is only written when the bus is configured with persist: true. The local server leaves that off by default: routes.ts passes options.events?.persist, and only workerd.ts sets it. In the default CLI the lookup finds nothing and falls back to message.seq, so the new "enqueued after the boundary but before its delivery" behavior only happens on some deployments. I confirmed this by switching the revert test layer to persist: false: the "after the boundary is promoted" test then fails. This also changes behavior for normal promoted boundaries, which #39736 doesn't need. I'd drop this lookup and keep the previous message.seq cutoff. If the wider cutoff is wanted, the enqueue seq should come from projected state (e.g. recorded on the inbox or message row) and go in a separate PR.
There was a problem hiding this comment.
Done in d96c284: the commit projection no longer reads the event log for the inbox cutoff — it keeps the message seq for promoted boundaries — and both tests covering that path are removed. I verified the problem first: with event persistence off (the default for the local server), the promoted-boundary test failed because the lookup found nothing. Session suites pass (125 tests across session-revert, session-prompt, session-projector, session-diff and session-owned) and bun run check is 33/33. One behaviour note, also in the PR body: input admitted after the boundary but before delivery is now kept when the boundary was promoted.
There was a problem hiding this comment.
Thanks, I checked d96c284. The commit projection no longer reads the event log: promoted boundaries go back to cutting the inbox at message.seq, so the result no longer depends on whether events are persisted. Keeping input admitted after a promoted boundary but before its delivery matches the behavior before this PR, so that's fine for this change. The removed tests resolve my test comment too. Nothing more from me on this thread.
| }), | ||
| ) | ||
|
|
||
| it.live("commits a revert staged while unpromoted after the boundary is promoted", () => |
There was a problem hiding this comment.
This test only passes because this file sets up the bus with persist: true, so it doesn't reflect the default local server (see the projector comment). If the event-log cutoff is dropped, this test can go too. The "keeps materialized assistant messages" test below hand-builds a Step.Started/Ended pair to check a case the issue doesn't describe; I'd drop it as well and keep the small tests for stage/commit on an unpromoted boundary, the compaction rejection and the other-session check.
…aries The commit projection recovered the inbox cutoff from the durable event log, which is only written when event persistence is on. The default local server leaves it off, so the lookup found nothing there and the wider cutoff only applied on some deployments. Keep the message seq as the inbox cutoff and drop the two tests that covered the promoted boundary cutoff, per opencodejr review.
There was a problem hiding this comment.
Both earlier points are fixed. The commit projection no longer reads the event log: promoted boundaries go back to cutting the inbox at message.seq, so behavior no longer depends on event persistence. The two tests I flagged are removed. At the new head, the session revert, prompt and projector tests pass (104), and packages/core type-checks. The fix for #39736 is unchanged from my earlier check, where the new tests failed on the base with MessageNotFoundError and passed with this PR.
Public API: unchanged. session.revert.stage/commit now also accept an admitted inbox ID that used to return "message not found".
No further concerns.
Issue for this PR
Closes #39736
Type of change
What does this PR do?
Reverting to a prompt that was sent but not yet shown failed with "message not found", because the core only looked the boundary up in the message history.
This resolves the boundary from the admitted input when it isn't in the message history yet, and committing such a revert removes just the pending input after it. For a boundary that was promoted between staging and committing, the inbox cutoff stays at the message seq, so input admitted after the boundary but before it was shown is kept: the enqueue seq is not recoverable from projected state on deployments without event persistence.
#58118 fixes the same issue from the client side; this is the matching core-side handling.
Disclosure: AI coding assistance was used in preparing this change.
How did you verify your code works?
Added 6 tests (5 in
session-revert.test.ts, 1 busy-session gate insession-prompt.test.ts). Before the fix, 3 of them failed with the reported error; after the fix all 6 pass. Ran the session suites (session-revert,session-prompt,session-projector,session-diff,session-owned): 125 pass, 0 fail.bun run checkpasses (33/33).Checklist
Added by the opencode bot (AI-generated).
Related: #57383