mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
🧹 fix: Codex round 8 — remove the unverifiable disarm signal
Round 8 found the same over-promise on the disarm side that round 7 corrected on the arm side, so this applies the same answer rather than patching around it. The `disarmed: false` field added in round 7 was both unreliable and unused: a resolved publish is not proof the owner heard it (the delivery count includes this replica's own facade subscription), and it was never threaded into `CancelSteerResponse` or read by any client. A signal that claims a certainty the transport cannot provide is worse than no signal — it invites callers to trust it. Removed from the response. The retry stays, because it genuinely reduces the failure rate, and `noteSteersRemoved` still returns whether the publish succeeded FOR LOGGING, now documented explicitly as "published without error", not "the owner disarmed". Disarm is best effort with a bounded, self-healing failure: if the clear is lost the owner seals once, the empty-boundary self-clear disarms the generation, and the turn is persisted `unfinished: true` rather than silently truncated. Tightening that further needs a correlated request/response over pub-sub with a timeout — noted on the PR as the deliberate boundary of this design rather than an oversight. 130 packages/api steering specs green.
This commit is contained in:
parent
81fc0297c5
commit
c9d3ee715e
2 changed files with 29 additions and 21 deletions
|
|
@ -363,19 +363,21 @@ export async function handleSteerCancel(
|
|||
return { status: 200, body: { removed } };
|
||||
}
|
||||
/**
|
||||
* Awaited: a dropped disarm leaves the owner armed for a steer that no
|
||||
* longer exists, costing one sealed-and-empty boundary — a visibly
|
||||
* truncated answer, not just a stale label.
|
||||
* Awaited so a failed disarm is retried and logged before the response,
|
||||
* but its outcome is deliberately NOT reported to the client.
|
||||
*
|
||||
* `removed` stays true regardless: the steer really did leave the queue,
|
||||
* and reporting otherwise would make the client re-show a chip for a steer
|
||||
* that can never arrive. `disarmed: false` surfaces the residual risk
|
||||
* without lying about the removal.
|
||||
* A resolved publish is not proof the owner heard it — the delivery count
|
||||
* includes this replica's own facade subscription — so any `disarmed` flag
|
||||
* would claim a certainty the transport cannot provide, which is the same
|
||||
* over-promise the `preempt` flag was corrected for. Disarm is best effort
|
||||
* with a bounded, self-healing failure: if the clear is lost the owner
|
||||
* seals once, its empty boundary self-clears, and the turn is persisted
|
||||
* `unfinished: true` rather than silently truncated.
|
||||
*
|
||||
* `removed` stays true because it is true — the steer really did leave the
|
||||
* queue, and inverting it would make the client re-show a chip for a steer
|
||||
* that can never arrive.
|
||||
*/
|
||||
const disarmed = await GenerationJobManager.noteSteersRemoved(
|
||||
streamId,
|
||||
[body.steerId],
|
||||
job.createdAt,
|
||||
);
|
||||
return { status: 200, body: { removed, ...(disarmed === false && { disarmed: false }) } };
|
||||
await GenerationJobManager.noteSteersRemoved(streamId, [body.steerId], job.createdAt);
|
||||
return { status: 200, body: { removed } };
|
||||
}
|
||||
|
|
|
|||
|
|
@ -3302,15 +3302,21 @@ class GenerationJobManagerClass {
|
|||
return Promise.resolve(true);
|
||||
}
|
||||
/**
|
||||
* Awaitable so a CANCEL can react to a failed disarm. A dropped clear is
|
||||
* worse than a dropped arm: the owner keeps a level-triggered request for
|
||||
* a steer that no longer exists, seals its next chunk, drains nothing,
|
||||
* and truncates an unrelated answer. Retried once — a transient publish
|
||||
* error is the common case and the retry is cheap — then reported to the
|
||||
* caller so it is not silently swallowed.
|
||||
* Awaitable so a CANCEL retries and logs before responding. A dropped
|
||||
* clear is worse than a dropped arm: the owner keeps a level-triggered
|
||||
* request for a steer that no longer exists, seals its next chunk,
|
||||
* drains nothing, and truncates an unrelated answer. Retried once — a
|
||||
* transient publish error is the common case and the retry is cheap.
|
||||
*
|
||||
* Damage is bounded even if both attempts fail: the empty-boundary
|
||||
* self-clear disarms the generation after that single seal.
|
||||
* The returned boolean means "published without error", NOT "the owner
|
||||
* disarmed": the delivery count includes this replica's own facade
|
||||
* subscription, so publication cannot prove receipt. It is for logging
|
||||
* only and must not be surfaced as a guarantee. Proving receipt needs a
|
||||
* correlated request/response over pub-sub.
|
||||
*
|
||||
* Damage is bounded regardless: if the clear is lost the owner seals
|
||||
* once, the empty-boundary self-clear disarms the generation, and the
|
||||
* turn is persisted `unfinished: true` rather than silently truncated.
|
||||
*/
|
||||
const publish = (): Promise<unknown> =>
|
||||
Promise.resolve(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue