fix(agent-core-v2): recover session persistence after disk-full instead of losing buffered records - #3695
fix(agent-core-v2): recover session persistence after disk-full instead of losing buffered records#3695LunarFeller wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 8e15f91 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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: 2d70e9c64f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| this.retryTimer = setTimeout(() => { | ||
| this.retryTimer = undefined; | ||
| this.scheduleFlush(); |
There was a problem hiding this comment.
When a write fails during session deletion, close() marks the journal closed but this callback still schedules a retry. purgeSession() awaits that close and then removes the journal in sessionEventBroadcaster.ts:550-557, so the delayed callback subsequently calls appendFile and recreates a journal containing data from the deleted session. Gate retry scheduling on !this.closed or explicitly cancel/discard the pending retry before deletion.
Useful? React with 👍 / 👎.
| const failure = (state.storageFailure ??= { error }); | ||
| if (state.recovery === undefined && isRecoverableStorageError(error)) { | ||
| state.recovery = { failedBatch: batch, attempts: 0, timer: undefined }; | ||
| this.scheduleRecovery(scope, key, state); |
There was a problem hiding this comment.
If the initial failure is retryable but a recovery append later encounters a permanent error, such as storage.permission_denied, state.storageFailure still contains the original disk-full/I/O error. This branch therefore throws that old retryable error, and attemptRecovery() classifies it as recoverable and schedules retries forever instead of becoming sticky on the current permanent failure. Update the stored failure during a recovery attempt or propagate the newly caught error for retry classification.
Useful? React with 👍 / 👎.
dc24c3b to
dff7d14
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dff7d142d6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const overlap = committedTailOverlap(tail, encoded); | ||
| if (overlap === encoded.byteLength) { | ||
| state.pending.splice(0, failedBatch.length); |
There was a problem hiding this comment.
When an append writes no bytes but the existing log already ends with bytes identical to the failed batch's prefix—for example, when the same record is appended twice—committedTailOverlap() reports those pre-existing bytes as a partial or full commit. This branch then removes that old suffix and rewrites it once, or splices the pending batch entirely, losing one logical record. Capture the file size before the append attempt so recovery only treats bytes beyond that boundary as part of the failed batch.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| this.pendingLines = (headerIncluded ? lines.slice(1) : lines).concat(this.pendingLines); | ||
| this.consecutiveFailures++; |
There was a problem hiding this comment.
If appendFile() writes only a prefix of the batch's first event line before rejecting, this restores that line and the retry appends it directly after the torn prefix. The resulting concatenated line is invalid JSON, so readLines() skips it and the first buffered event is permanently lost even after storage recovers. Truncate or terminate any torn tail before replaying the restored lines.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| this.pendingLines = (headerIncluded ? lines.slice(1) : lines).concat(this.pendingLines); | ||
| this.consecutiveFailures++; |
There was a problem hiding this comment.
If a failed append committed one or more complete lines, restoring the entire batch creates duplicate sequence numbers. In the inspected resync path, readSince() counts every physical line toward limit, while getBufferedSince() passes maxBufferSize and wsConnectionV1.replay() then advertises the journal's full currentSeq; duplicates can therefore fill the page and omit later unique events that the client is told it has caught up past. Reconcile the committed prefix or deduplicate by sequence before applying the limit.
Useful? React with 👍 / 👎.
…ad of losing buffered records An ENOSPC append failure put AppendLogStore into a sticky failure state that never retried, so all subsequently appended records lived only in memory and were lost on process exit. The store now retries in the background with backoff after reconciling the on-disk tail against the failed batch (never duplicating or rewriting user data), close/retirement make a final recovery attempt, and recovery failures are classified by the latest storage error. SessionEventJournal no longer drops buffered events (or the pending header) on a failed write, retries with backoff, and stops retrying once closed.
…nd boundary Suffix-prefix overlap matching could mistake pre-existing identical content for a committed batch and drop a duplicate-valued record. The drain now records the file size before each append attempt, and recovery reconciles against that exact boundary: zero committed bytes retries directly, a verified partial commit is trimmed back, a verified full commit is dropped from pending, and anything else stays sticky.
…d write A partially committed append used to duplicate complete event lines on retry and let a torn prefix swallow the first line of the retried batch, losing that event. The journal now measures what actually committed, restores only the lines that never made it, and prepends a newline on the next write when the tail was torn.
dff7d14 to
8e15f91
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e15f911f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| state.recovery = { | ||
| failedBatch: state.pending.slice(), | ||
| failedAtSize: 0, |
There was a problem hiding this comment.
When rewrite() fails with a recoverable error while state.pending is nonempty—for example, because an append arrives during the atomic rewrite—this creates append-recovery state with boundary zero even though failedBatch was never appended. Recovery consequently compares the entire existing or replaced log with the pending batch; a mismatch strands all pending records, while identical bytes cause them to be spliced as already committed and lose a logical append. Fresh evidence beyond the earlier append-boundary fix is this rewrite-only hard-coded zero; rewrite failure should instead heal the sticky state and drain pending records without append-tail reconciliation.
Useful? React with 👍 / 👎.
| if (!committedText.endsWith('\n')) this.repairNewline = true; | ||
| if (headerIncluded && completeLines > 0) this.headerPending = false; | ||
| this.pendingLines = lines.slice(completeLines).concat(this.pendingLines); |
There was a problem hiding this comment.
When one failed write leaves a torn event and sets repairNewline, a subsequent retry prepends a synthetic newline to batchText. If that retry writes only this newline before failing again, completeLines becomes one and this slice removes the first actual event even though none of its bytes were committed; the torn line is then skipped during replay and that sequence is permanently lost. Count the repair delimiter separately before slicing lines.
Useful? React with 👍 / 👎.
| async function readRange(filePath: string, start: number, end: number): Promise<string> { | ||
| let out = ''; | ||
| const stream = createReadStream(filePath, { encoding: 'utf8', start, end }); | ||
| for await (const chunk of stream) { |
There was a problem hiding this comment.
When a partial append ends inside a multibyte UTF-8 character in an event payload, stat() supplies byte offsets but this range is decoded as text, producing a replacement character at the torn boundary. The later startsWith comparison therefore fails, restores the whole batch without setting repairNewline, and the retry concatenates valid JSON directly onto the torn line, causing the event to be skipped. Fresh evidence beyond the earlier torn-tail finding is this new decoding of a byte-bounded prefix; read and compare buffers against the UTF-8 encoded batch instead.
Useful? React with 👍 / 👎.
| const batch = state.pending.slice(); | ||
| const sizeBefore = await this.storage.size(scope, key); | ||
| try { |
There was a problem hiding this comment.
If the newly added storage.size() call itself fails with a transient storage.io_failed, the exception occurs outside the recovery try block, so neither storageFailure nor a retry timer is installed. The auto-flush reports the error once and leaves the records buffered indefinitely until an unrelated append, read, or shutdown triggers another flush, despite transient I/O errors being explicitly recoverable. Handle the boundary lookup failure before attempting the append and schedule the same backoff retry.
Useful? React with 👍 / 👎.
1 participant
Related Issue
Resolve #2902
Note: a maintainer
/approvewas requested in the issue (with a fuller diagnosis and this fix's design) and is still pending — opening ahead of it so the change is reviewable; happy to hold or adjust per maintainer feedback.Problem
A single ENOSPC on the session wire log permanently disabled persistence for the rest of the process lifetime:
AppendLogStorerecorded a sticky failure and never retried, so every record appended after the first failure lived only in memory, with no user-facing signal — until an RPC that awaits the flush (e.g. resuming a session in the web UI) started failing with 50001. A later process restart then silently lost everything buffered since the first failure. (Observed in the wild: ~12.5 hours of session history lost after a ~2-minute full-disk window; the OS had freed space again within two minutes.)The session event journal used by the web UI resync path also dropped buffered events on a failed write, and lost the journal header when the first write failed (a header-less file forces an epoch rotation on reopen).
What changed
AppendLogStore: astorage.disk_full(or retryablestorage.io_failed/storage.locked) append failure now enters a degraded mode instead of permanent stickiness: pending records stay buffered, the failure is reported once per episode (no more[unexpected]log spam per append), and a backoff retry loop (1s → 30s cap) retries in the background. Before each retry it reconciles the on-disk tail against the failed batch: full commit → drop the batch without rewriting; strict-prefix partial commit → atomically trim the torn tail, then resume; mismatched or foreign torn tail → stay sticky, never rewrite user data. The reconcile reads only the file's tail window, not the whole log.close()and log retirement make one immediate recovery attempt, so a graceful shutdown flushes buffered records once space is back. A rewrite failure caused by a full disk now also schedules recovery (it previously ended it). NewonDidRecoverevent;WireServicelogs an actionable degraded message and a recovery info line.SessionEventJournal(kap-server): failed writes restore buffered lines instead of dropping them, the journal header stays pending until actually written, and retries use backoff instead of an immediate rescheduling loop.flush()still drains to quiescence for readers, but stops after a failed attempt (the backoff timer owns the rest) soreadSince()/close()can neither spin nor hang during an outage.Rejected alternatives: blind retry after a failed append (violates the deliberate no-retry-after-ambiguous-commit contract — it can duplicate records or leave a torn line); retrying a failed
rewrite()inside the store (the atomic write already fails safe, and callers such as wire repair re-derive and retry the content themselves — the store-side recovery here only heals the sticky state so future appends flow again).Exactly-once recovery: the wire-side reconcile is anchored at the original append boundary (file size recorded before each attempt), so it never mistakes pre-existing content for a committed batch. The journal likewise measures what actually committed after a failed write, restores only the uncommitted lines, and repairs a torn tail with a leading newline — no duplicate or swallowed events (found by Codex auto-review, fixed with regression tests).
Not included (noted as follow-ups in the issue): recording assistant content in the event journal; a "persistence degraded" banner in the web UI (needs the web repo).
Checklist
/approverequested there and currently pending).gen-changesetsskill —.changeset/rich-pandas-heal.md(patch).gen-docsskill — no user-facing doc update needed (internal persistence behavior only).