fix(mothership): number recovered turns past an unreadable ring and floor the replay TTL - #8501
Conversation
…loor the replay TTL A recovered run whose replay ring read back empty while its seq counter survived (every retained entry unreadable) resumed numbering at 0, writing over the old range and moving the counter backwards past readers' cursors. Resume from the counter whenever no event was recovered. COPILOT_STREAM_TTL_SECONDS below the 20 s chat-lock heartbeat let an idle live buffer expire between refreshes. Floor it at 60 s.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
…uffer floor check
…y-seq-and-ttl-floor
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
This branch was successfully deployed
1 participant
Summary
This PR fixes two Chat stream-recovery findings from the release review. Neither one is a regression against the current release, and neither blocks it.
1. A recovered turn can number its events at 0 over an unreadable ring (
recover-stream.ts)Release-review finding: a recovery whose
readEventscomes back empty resumes numbering at 0, even though the stream's seq counter has survived.Root cause.
readEventsskips any ring entry that fails envelope parsing. If every retained entry is unreadable,eventsis empty.startsAtReplayHead(undefined)is true, so the code took the "ring intact" branch withresumeSeq = events.at(-1)?.seq ?? 0 = 0. The writer then numbers new events from 1. The append script ZADDs them next to the old scores andSETs the counter, which moves it backwards (N → k). A live tail whose cursor sits between the old and new numbering can then silently skip new events. The empty-events path predates the lost-head branch added in #8469; prod has the same behaviour.Fix. Use the recovered event's seq only when an event was actually recovered. Otherwise resume from the stream's counter (
getLatestSeq), which still falls back to 0 when the key is missing. The lost-head branch already took this path. The controller still starts from an empty context and re-attaches with an empty receipt, so nothing else changes.2. A configured replay TTL below the heartbeat lets an idle live buffer expire (
buffer.ts)Release-review finding: the 20 s chat-lock heartbeat refreshes the live buffer's TTL, but
COPILOT_STREAM_TTL_SECONDSaccepted any value of 1 s or more.Root cause. With a TTL under about 20 s, a run that goes quiet loses its events and seq keys between refreshes. The refresh only calls
EXPIRE, so it cannot bring them back. The default (3600 s) is unaffected, and nothing in the repo sets the variable.Fix.
getStreamConfigfloors the configured live TTL at 60 s, which is three heartbeats. The default and every value of 60 s or more behave as before. The completed-stream TTL is unchanged.Behaviour changes
zrangebyscore, so the gap is harmless.COPILOT_STREAM_TTL_SECONDSvalues below 60 now act as 60.Not changed
max(lastRecoveredSeq, counter), would also cover a corrupt tail behind a readable head. I left it out: the finding was the empty-ring case, and that variant adds a Redis read to every recovery for a case that needs corrupt data mid-ring.Test plan
stream-recovery.integration.ts: new case, a ring whose entries are all unreadable recovers with new events numbered after the counter (min seq 5). Fails on the pre-fix code.buffer-ttl.integration.ts: new case, a live buffer under a 5 s configured TTL gets at least 60 s. Fails on the pre-fix code. The existing park test now shortens the event and seq keys' TTLs explicitly, since the floor no longer lets the env do it.bun run test:integration buffer-ttl.integration.ts stream-recovery.integration.ts, disposable Postgres/Redis).recover-stream.test.tsand thesession/unit tests pass.bun run lint,bun run type-check(apps/sim),bun run check:auditspass.bun run test:integration(left to CI).