fix(agent-core-v2): recover session persistence after disk-full instead of losing buffered records - #3695
Conversation
…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), and close/retirement make a final recovery attempt. SessionEventJournal no longer drops buffered events (or the pending header) on a failed write and retries with backoff.
🦋 Changeset detectedLatest commit: dff7d14 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 👍 / 👎.
A retry armed by a failed write could fire after the journal was closed and recreate a deleted session's journal file. Retry scheduling is now gated on the journal still being open.
…error A recovery attempt that failed with a different, permanent error (e.g. permission denied) used to be classified by the original disk-full error and retried forever. The latest failure now replaces the stored one.
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 👍 / 👎.
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).Known trade-off: the event journal has no truncate primitive, so a retry after a partially committed write can produce duplicate (well-formed) event lines on disk. Readers tolerate this (torn lines are skipped, the first header wins, transcript ops are idempotent) — strictly better than the previous silent loss.
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).