fix: make CloudAppender shutdown durable - #2574
Conversation
…AI#2252) - Serialize flush() to prevent concurrent buffer races - Add deadline-bounded shutdown() with AbortController - Hand unsent events to durable storage before completing - Replay v2 spool data for at-least-once delivery - Update CloudTransport to support AbortSignal - Update kap-server telemetry to use new shutdown API - Add comprehensive test coverage for shutdown scenarios Fork main has been synced to upstream (071b6a5). Original PR MoonshotAI#2252 was closed due to fork being 48 commits behind.
🦋 Changeset detectedLatest commit: 59199bc 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: a905b8173d
ℹ️ 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".
| } | ||
|
|
||
| try { | ||
| await this.flush().catch(() => {}); |
There was a problem hiding this comment.
When shutdown overlaps a timer/threshold flush that already entered doFlush, that send was started before shutdownController existed, so it has no abort signal; this await this.flush() first waits for that old flushInFlight, and packages/kap-server/src/services/telemetry.ts now relies solely on appender.shutdown(deadlineMs) instead of its previous Promise.race. A wedged telemetry request can therefore keep server close blocked well past the 3s deadline; keep an outer timeout or make already-started sends abortable.
Useful? React with 👍 / 👎.
| client_version: bootstrap.clientVersion, | ||
| version: bootstrap.clientVersion, |
There was a problem hiding this comment.
In production/server initialization via createCloudAppender, bootstrap is the real IBootstrapService, whose host version lives at bootstrap.clientIdentity.version; clientVersion is only added by the updated unit-test stub. These lines therefore serialize undefined/omit the client version from every cloud telemetry payload, so use bootstrap.clientIdentity.version for both context fields.
Useful? React with 👍 / 👎.
| }); | ||
| }); | ||
|
|
||
| describe('shutdown durability', () => { |
There was a problem hiding this comment.
Because this new describe starts after the outer describe('CloudAppender') closes, the tests no longer share the let homeDir plus beforeEach/afterEach setup defined inside that suite. Each new case that passes homeDir will hit ReferenceError: homeDir is not defined when executed, so move this block back inside the outer suite or hoist the fixture setup.
Useful? React with 👍 / 👎.
| * Shutdown lifecycle: | ||
| * - `flush()` is guarded by a chained-promise lock so concurrent periodic / | ||
| * threshold / manual / shutdown triggers serialize without two waiters | ||
| * racing on a cleared `flushInFlight` flag. | ||
| * - `shutdown(deadlineMs?)` provides a deadline-bounded, idempotent close: |
There was a problem hiding this comment.
For this agent-core-v2 file, the added shutdown lifecycle block documents the chained lock, AbortController deadline, spool replay, and delivery mechanics inside the module header. The scoped guide says top-of-file comments should state the external role and not narrate implementation steps, so this should move to code/tests or be removed to keep headers stable.
AGENTS.md reference: packages/agent-core-v2/AGENTS.md:L15-L20
Useful? React with 👍 / 👎.
When shutdown() overlaps a timer/threshold flush that already entered doFlush(), the in-flight send was started before shutdownController existed and has no abort signal. A hung telemetry request could block server close past the deadline. Add Promise.race to ensure flush() is bounded by remaining deadline, regardless of whether the in-flight send respects the abort signal.
- Use bootstrap.clientIdentity.version instead of bootstrap.clientVersion (IBootstrapService exposes clientIdentity, not clientVersion directly) - Move 'shutdown durability' describe block inside outer CloudAppender suite so tests share homeDir fixture and beforeEach/afterEach setup - Remove implementation-detail narrative from module header per AGENTS.md scoped guide (headers state external role, not internal steps)
1 participant
Make CloudAppender shutdown durable:
This replaces closed PR #2252. Fork main has been synced to upstream (071b6a5).
Original PR was closed because the fork was 48 commits behind upstream main.