Conversation
codegraph-librechat
Bot
added
the
🗺️ Backend Platform
f0336da to
fbaf453
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbaf453a9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| jobData?.userMessage?.messageId && | ||
| jobData?.responseMessageId && | ||
| shouldPersistAbortedTurn | ||
| abortPersistencePlan.writeResponseRow |
There was a problem hiding this comment.
When a compaction with a persisted anchor is stopped, this branch writes the response with unfinished: true at line 821. Since abortJob has already won terminal ownership, the request controller exits without a later final save, so the row remains unfinished after reload and the new E2E assertion in compaction-abort-finalize.spec.ts:102 cannot pass. Persist compaction aborts with a settled envelope while retaining the unfinished behavior for ordinary stopped replies.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c95e57e. The abort plan now carries responseUnfinished (false for a compaction, true for an ordinary turn) and the route writes it. The e2e scenario stopped-compaction-settles-under-its-anchor reproduced this on fbaf453 (stored row unfinished: true on all three projects) and passes on 170672d; stopped-reply-keeps-turn-and-partial-answer covers the ordinary path.
| const abortPersistencePlan = planAbortedTurnPersistence( | ||
| await resolveAbortedTurnAnchorDecision(jobData, { | ||
| messageExists: (messageId, conversationId) => | ||
| getMessages({ user: req?.user?.id, messageId, conversationId }, '_id').then( | ||
| (rows) => rows.length > 0, | ||
| ), | ||
| }), |
There was a problem hiding this comment.
This adds backend behavior directly to the legacy route: it performs the anchor database read, interprets the result, and converts it into terminal-publication failure state. The repository boundary requires branches and service calls added under /api to live in packages/api, with the CJS file limited to route wiring and a call into the TypeScript module; move this DB-backed orchestration behind an injected TypeScript service rather than splitting the invariant across both layers.
AGENTS.md reference: AGENTS.md:L77-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c95e57e. resolveAbortedTurnPersistence in packages/api now takes the injected getMessages, does the id-only anchor read, plans the rows and returns the persistence errors; the route makes one call and spreads persistenceErrors.
| if (coveredByExistingRow) { | ||
| return; |
There was a problem hiding this comment.
When a disconnected compaction has already saved a partial row and generation then fails, settleExistingRowsBeforeErrorTurn finalizes that row and this early return skips both saveConvo and announceErrorTurn below. Consequently lastResponseAt is not stamped and another device has no notification that the persisted turn ended, even though the ordinary error-row path explicitly performs that announcement. Announce the finalized live row before returning so failure and restored-session behavior remain consistent.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c95e57e. settleExistingRowsBeforeErrorTurn takes an injected announceSettledTurn and calls it only when it actually finalized the live row; request.js passes announceErrorTurn with stampConvoLastResponse. compaction.spec.ts covers both the announce and the no-write case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 170672dff0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try { | ||
| const anchorExists = await messageExists(anchorId, jobData.conversationId); | ||
| return anchorExists ? 'skip-anchor' : 'skip-turn'; | ||
| } catch { | ||
| return 'skip-turn'; |
There was a problem hiding this comment.
When the injected message lookup rejects, this catch converts a database outage into the same skip-turn result as a confirmed missing anchor. The caller then discards the original operational error, reports only the synthetic “anchor unavailable” persistence failure, and returns a successful abort with no stopped row, preventing the job boundary and logs from distinguishing an outage from absence. Let the rejection reach the existing beforePublish failure boundary or preserve it in a typed result.
AGENTS.md reference: AGENTS.md:L116-L121
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3650e89. The read still resolves to skip-turn so checkpoint cleanup runs, but resolveAbortedTurnPersistence now returns the original read error first in persistenceErrors, which the route throws at the beforePublish boundary, so an outage stays distinguishable from an absent anchor. compaction.spec.ts asserts the outage object reaches persistenceErrors.
| if ((!hasOutcome || removedUnfinishedRound) && synthesizeFailure) { | ||
| contentParts.push(...compactionFailureContent()); |
There was a problem hiding this comment.
When content contains an empty placeholder from an earlier failed or retried round followed by a usable summary from a later round, removing the older placeholder sets removedUnfinishedRound, and this condition appends COMPACTION_FAILED even though the latest summary completed before Stop. Multiple summary rounds are supported and checkpoints are last-summary-wins, so this produces a false failure beside a successful compaction; only a removed round after the latest usable outcome should trigger the synthesized failure.
AGENTS.md reference: AGENTS.md:L49-L52
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a5c4ac3. Walking backwards, a removed placeholder now counts only when no later part has already supplied an outcome, so an earlier abandoned round superseded by a later usable summary no longer gets a synthesized failure. compaction.spec.ts adds that case; the later-empty case still records the failure.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Pull Request
Summary
Related to berry-13#33. Stacked on #16436, which marks a stopped manual compaction with its identity; review that one first.
With only the identity marking in place, a compaction's disconnect snapshot could become the turn's final row: when the run errored afterwards,
saveErrorTurnreturned early on the existing partial row and never applied the terminal outcome, so the row kept its liveunfinishedshape. The abort route also skipped the anchor upsert unconditionally, so a Stop that won before the branch loaded wrote a response parented on a row that was never persisted, and then published a normal FINAL for it.This moves the stopped turn's persistence into one
planAbortedTurnPersistenceoperation inpackages/api, which verifies the compaction anchor with an id-only read, withholds the final (publishing a reconciliation frame) only when a row needed writing and could not be anchored, and keeps checkpoint cleanup running when the anchor read fails. The failed-turn settlement also moves into the compaction module: an existing snapshot is finalized with the typed failure and error envelope, falsy writes are rejected, an anchor-shaped error id is never mistaken for the anchor, and snapshots are not written over a settled job.Type of change
Testing
Tested environments/configuration: jest for the unit, controller and route suites; MongoDB and the in-memory job store.
Automated tests:
packages/api:npx jest src/agents/compaction.spec.ts src/stream/__tests__/abortCompactionIdentity.spec.tsplussrc/stream/__tests__/abortCompactionIdentity.spec.ts src/stream/__tests__/RedisJobStore.spec.ts: 130 passednpx tsc --noEmit -p packages/api/tsconfig.json: cleanapi:npx jeston the abort route, request partial disconnect and resume metadata suites: 194 passedScreenshots / recordings
No user-facing change.
Risk / compatibility
Ordinary (non-compaction) aborts keep the anchor upsert and their FINAL. The abort route adds one id-only anchor read for compaction jobs.
Checklist