Repository navigation
fix(core): map imported message-ID collisions to a 409 conflict - #54353
eatmoreduck wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
needs:compliance
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix looks good. I ran the reproduction from #54336 against both checkouts with the simulated model: export a session, change info.id, import the copy. On the base the import returns HTTP 500. With this change it fails with ConflictError (409), and no session is created for the new ID. The new server test fails on the base and passes here. The other session-import tests, the core session-create tests, and type checks for core and server all pass.
The public API is unchanged: the import endpoint now returns its existing 409 conflict instead of a 500.
No blocking concerns. The check runs before the write transaction, so a concurrent insert, or a payload that repeats a message ID within itself, could still give a 500. Both are rare and don't need to hold this up.
Related: #50260
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
One note from the original reproduction (matching the triage suggestion on #54336): with An assertion that the rejected import did not create the destination session is also worth adding — that is the failure mode when the constraint fires. |
|
Both done in 1783849: |
|
Thanks for picking this up! I had a PR (#54543) fixing the same issue (#54336) that was closed as a duplicate of this one. One suggestion: consider a dedicated |
|
Done in e2da1ce — |
e2da1ce to
d29594d
Compare
There was a problem hiding this comment.
The fix works, but the latest commit breaks the test file.
I ran the #54336 reproduction against both checkouts with the simulated model: export a session, change info.id, import the copy. On the base it returns a 500. With this change it returns a 409 ConflictError ("Session messages already exist"), and no session is created for the new ID.
The last commit adds two stray lines, ).pipe(Effect.scoped), and ), at lines 53–54 of packages/server/test/session-import.test.ts. Because of them, bun test test/session-import.test.ts stops with a parse error and runs no tests, and bun run typecheck in packages/server fails (TS1128/TS1109). With those two lines removed, all 6 import tests pass and type checks for server and core are clean.
Public API: the response shape is unchanged. A message-ID collision now returns the existing 409 ConflictError, with the new message Session messages already exist: <id>, instead of a 500.
packages/server/test/session-import.test.ts:53: These two lines are left over from an edit: the test above is already closed on lines 51–52. They are a syntax error, sobun teststops before running anything in this file andbun run typecheckfails with TS1128/TS1109. Removing them makes all 6 tests pass.
There was a problem hiding this comment.
The stray lines in session-import.test.ts are gone. The import tests (6/6) and the server type check pass again. I re-ran the #54336 reproduction with the simulated model: the base still returns a 500, and this change returns a 409 ConflictError without creating the new session.
Public API: the response shape is unchanged. A message-ID collision now returns the existing 409 ConflictError instead of a 500.
No remaining concerns.
Importing a session whose message IDs already exist (e.g. re-importing an export under a new session ID) hit the session_message UNIQUE constraint and died as a 500 with an empty body. Pre-check the settled message IDs (batched at 500, same as stats.ts) before any write and fail with the existing SessionTransfer.ImportConflictError, which the HTTP handler already maps to 409. Fixes anomalyco#54336
…ssions ImportConflictError now carries a reason: "session" keeps the existing 'Session already exists' body, while "messages" (only message IDs taken, the session row was never created) answers 'Session messages already exist: <id>'. The tests now assert both 409 bodies and that a rejected message-ID import leaves no destination session behind.
…t conflicts Per review: a message-ID conflict never created the destination session, so reusing ImportConflictError (with a reason discriminant) made every consumer unwrap a union that only this path produced. MessageConflictError now carries the sessionID directly and maps to the 409 'Session messages already exist: <id>' body; ImportConflictError is back to meaning 'the session row exists'. Both 409 bodies and the no-destination-session assertion are covered by the import tests.
The conflict resolution left a duplicate ').pipe(Effect.scoped),' and ')' at the top of the file; bun test parsed nothing and typecheck failed (TS1128/TS1109). With them removed the 6 import tests pass.
bfe9f1d to
205627c
Compare
|
Rebased onto current dev — the conflict was upstream's new import subpath feature crossing our settled-messages refactor; resolution keeps both (subpath computation retained, messages built from the existing |
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Closes #54336
Type of change
What does this PR do?
session importcurrently dies with HTTP 500 (empty body) when the payload's message IDs already exist insession_message— reachable by re-importing an export under a new session ID, or when orphaned message rows survive their session (#50260).SessionTransfer.importonly checked the session row for duplicates, then inserted messages with their original IDs behindEffect.orDie, so the UNIQUE constraint surfaced as a defect. This pre-checks the settled message IDs (batched at 500, same pattern asstats.ts) before any write and fails with the existingSessionTransfer.ImportConflictError, which the HTTP handler already maps to 409.How did you verify your code works?
packages/server/test/session-import.test.ts: importing a payload whose message IDs exist under another session returns 409ConflictErrorand does not create the target session — red ondev, green with this changedevbaseline (remaining failures are pre-existing environment-dependent tests: shell/bash-3.2, MCP network, OAuth ports)bun run typecheckandoxlintclean on changed filesScreenshots / recordings
Not a UI change.
Checklist
This change was made with AI assistance (GLM via ZCode agent, under my direction); I reviewed and verified the full diff, reproduction, and test results locally.
Added by the opencode bot (AI-generated).
Related: #50260