Fix saved package publish artifact rebuild memory pressure - #440
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a build-target abstraction and RPC endpoints to list/rebuild published bundle artifact targets, implements a repo-session rebuild helper that sequentially rebuilds targets, and integrates post-publish artifact rebuilds into external-push, publish-session, and run-commands flows; tests and package-registry loading were updated for optional rebuild skipping. ChangesPublished Package Artifact Rebuild Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Handler as Publish Handler
participant RepoRPC as RepoSessionRpc
participant DO as RepoSessionDO
participant Registry as Package Registry
participant KV as KV Store
Handler->>RepoRPC: publishFromExternalRef(..., rebuildPackageArtifacts:false)
RepoRPC->>DO: delegate publishFromExternalRef
DO-->>RepoRPC: status published|already_published + published_commit
Handler->>RepoRPC: listPublishedPackageArtifactTargets({sessionId?, sourceId, userId})
RepoRPC->>DO: delegate listPublishedPackageArtifactTargets
DO-->>RepoRPC: targets[]
loop for each target
Handler->>RepoRPC: rebuildPublishedPackageArtifact({target, publishedCommit, baseUrl?, sessionId?})
RepoRPC->>DO: delegate rebuildPublishedPackageArtifact
DO->>KV: persistPublishedPackageArtifactTarget(...)
KV-->>DO: kvKey
DO-->>RepoRPC: {ok:true, target, kvKey}
RepoRPC-->>Handler: {ok:true, target, kvKey}
end
Handler->>Handler: return result with rebuilt artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
May 11, 2026 23:33
Contributor
|
🔎 Preview deployed: https://kody-pr-440.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts (1)
90-108: 💤 Low valueConsider refactoring nested ternary for readability.
The three-level nested ternary logic (lines 90-108) works correctly but is cognitively complex. Consider extracting this into a helper function or using if-else blocks for better maintainability.
♻️ Example refactor with if-else blocks
-const publish = - args.publish === true - ? result.checks.status === 'failed' - ? { - status: 'blocked_by_checks' as const, - message: 'Publishing skipped because repo checks failed.', - failedChecks: result.checks.failedChecks, - runId: result.checks.runId, - treeHash: result.checks.treeHash, - checkedAt: result.checks.checkedAt, - } - : result.checks.status === 'passed' - ? await sessionRpc.publishSession({ - sessionId: validatedSession.id, - userId: user.userId, - rebuildPackageArtifacts: false, - }) - : { status: 'not_requested' as const } - : result.publish +let publish +if (args.publish === true) { + if (result.checks.status === 'failed') { + publish = { + status: 'blocked_by_checks' as const, + message: 'Publishing skipped because repo checks failed.', + failedChecks: result.checks.failedChecks, + runId: result.checks.runId, + treeHash: result.checks.treeHash, + checkedAt: result.checks.checkedAt, + } + } else if (result.checks.status === 'passed') { + publish = await sessionRpc.publishSession({ + sessionId: validatedSession.id, + userId: user.userId, + rebuildPackageArtifacts: false, + }) + } else { + publish = { status: 'not_requested' as const } + } +} else { + publish = result.publish +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts` around lines 90 - 108, The nested ternary assigning publish is hard to read—replace it with a small helper or if/else block that computes publish based on args.publish and result.checks.status; for example create a function (e.g., computePublishPayload or determinePublishStatus) that takes args.publish, result.checks and validatedSession/user and returns either the blocked_by_checks object (including failedChecks, runId, treeHash, checkedAt), calls sessionRpc.publishSession when status === 'passed', or returns { status: 'not_requested' } when checks are neither passed nor failed, and fallback to result.publish when args.publish !== true; update the const publish = ... line to call that helper so the logic around args.publish, result.checks.status, and sessionRpc.publishSession is clear and linear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts`:
- Around line 109-123: The call to
rebuildPublishedPackageArtifactsViaRepoSession lacks error handling which can
leave artifacts stale; wrap the invocation inside a try-catch in the
repo-run-commands capability (the block where args.publish === true &&
publish.status === 'ok' && validatedSession.entity_type === 'package') and
handle failures by either (a) logging the error with context (include
validatedSession.id, publish.publishedCommit, and user.userId) and returning a
partial success response/flag indicating artifacts need manual repair, or (b)
re-throwing a new error that annotates "publish succeeded but artifact rebuild
failed" so upstream callers can distinguish the two outcomes; ensure the catch
uses processLogger (or existing logger in ctx) and preserves the original error
details when re-throwing or returning the partial status.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts`:
- Around line 90-108: The nested ternary assigning publish is hard to
read—replace it with a small helper or if/else block that computes publish based
on args.publish and result.checks.status; for example create a function (e.g.,
computePublishPayload or determinePublishStatus) that takes args.publish,
result.checks and validatedSession/user and returns either the blocked_by_checks
object (including failedChecks, runId, treeHash, checkedAt), calls
sessionRpc.publishSession when status === 'passed', or returns { status:
'not_requested' } when checks are neither passed nor failed, and fallback to
result.publish when args.publish !== true; update the const publish = ... line
to call that helper so the logic around args.publish, result.checks.status, and
sessionRpc.publishSession is clear and linear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1da5d0a0-5e0a-4dd0-be7d-4c03e676243b
📒 Files selected for processing (11)
packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.tspackages/worker/src/mcp/capabilities/repo/package-artifact-rebuild.tspackages/worker/src/mcp/capabilities/repo/repo-publish-session.tspackages/worker/src/mcp/capabilities/repo/repo-run-commands.tspackages/worker/src/mcp/capabilities/repo/repo-workflow.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-runtime/published-bundle-artifacts.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/repo-session-rpc.ts
Comment on lines
+109
to
+123
| if ( | ||
| args.publish === true && | ||
| publish.status === 'ok' && | ||
| validatedSession.entity_type === 'package' | ||
| ) { | ||
| await rebuildPublishedPackageArtifactsViaRepoSession({ | ||
| env: ctx.env, | ||
| rpcSessionId: validatedSession.id, | ||
| sessionId: validatedSession.id, | ||
| sourceId: validatedSession.source_id, | ||
| userId: user.userId, | ||
| publishedCommit: publish.publishedCommit, | ||
| baseUrl: ctx.callerContext.baseUrl, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add error handling for artifact rebuild failures.
The call to rebuildPublishedPackageArtifactsViaRepoSession has no try-catch. If artifact rebuilding fails, the capability returns a success response but artifacts remain stale or missing. This creates inconsistent state where D1 points at the new commit but KV bundles are out of sync.
Consider wrapping the rebuild in a try-catch and either:
- Log the error and return a partial success status indicating artifacts need manual repair, or
- Re-throw with context so the caller knows the publish succeeded but artifact rebuild failed.
🛡️ Proposed error handling
if (
args.publish === true &&
publish.status === 'ok' &&
validatedSession.entity_type === 'package'
) {
+ try {
await rebuildPublishedPackageArtifactsViaRepoSession({
env: ctx.env,
rpcSessionId: validatedSession.id,
sessionId: validatedSession.id,
sourceId: validatedSession.source_id,
userId: user.userId,
publishedCommit: publish.publishedCommit,
baseUrl: ctx.callerContext.baseUrl,
})
+ } catch (error) {
+ console.error('Failed to rebuild published package artifacts:', error)
+ throw new Error(
+ `Published commit ${publish.publishedCommit} but artifact rebuild failed. Artifacts may need manual repair.`,
+ { cause: error }
+ )
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts` around lines
109 - 123, The call to rebuildPublishedPackageArtifactsViaRepoSession lacks
error handling which can leave artifacts stale; wrap the invocation inside a
try-catch in the repo-run-commands capability (the block where args.publish ===
true && publish.status === 'ok' && validatedSession.entity_type === 'package')
and handle failures by either (a) logging the error with context (include
validatedSession.id, publish.publishedCommit, and user.userId) and returning a
partial success response/flag indicating artifacts need manual repair, or (b)
re-throwing a new error that annotates "publish succeeded but artifact rebuild
failed" so upstream callers can distinguish the two outcomes; ensure the catch
uses processLogger (or existing logger in ctx) and preserves the original error
details when re-throwing or returning the partial status.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bfc4dd3. Configure here.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
cursor
Bot
force-pushed
the
cursor/kody-package-publish-memory-bfb2
branch
from
May 12, 2026 00:48
e60b579 to
a577f5e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/repo/package-artifact-rebuild.ts (1)
19-31: ⚡ Quick winReuse the session RPC client instead of creating a new one per iteration.
Lines 20-22 create a fresh
repoSessionRpcclient for each artifact target, but thesessionclient initialized on line 13 can be reused, reducing overhead.♻️ Proposed refactor to reuse the session client
for (const target of targets) { - await repoSessionRpc( - input.env, - input.rpcSessionId, - ).rebuildPublishedPackageArtifact({ + await session.rebuildPublishedPackageArtifact({ sessionId: input.repoSessionId, sourceId: input.sourceId, userId: input.userId, publishedCommit: input.publishedCommit, target, baseUrl: input.baseUrl, }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/mcp/capabilities/repo/package-artifact-rebuild.ts` around lines 19 - 31, The loop currently calls repoSessionRpc(input.env, input.rpcSessionId) for each target, creating a new client per iteration; instead reuse the existing session client (the variable named session initialized earlier) by calling session.rebuildPublishedPackageArtifact(...) inside the loop with the same payload (sessionId: input.repoSessionId, sourceId: input.sourceId, userId: input.userId, publishedCommit: input.publishedCommit, target, baseUrl: input.baseUrl), so remove the per-iteration repoSessionRpc(...) call and use session to avoid repeated client creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/repo/package-artifact-rebuild.ts`:
- Around line 19-31: The loop currently calls repoSessionRpc(input.env,
input.rpcSessionId) for each target, creating a new client per iteration;
instead reuse the existing session client (the variable named session
initialized earlier) by calling session.rebuildPublishedPackageArtifact(...)
inside the loop with the same payload (sessionId: input.repoSessionId, sourceId:
input.sourceId, userId: input.userId, publishedCommit: input.publishedCommit,
target, baseUrl: input.baseUrl), so remove the per-iteration repoSessionRpc(...)
call and use session to avoid repeated client creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 220ed3b2-1b81-4652-bdb7-fd1de4ed507a
📒 Files selected for processing (13)
packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.tspackages/worker/src/mcp/capabilities/packages/publish-external-push.tspackages/worker/src/mcp/capabilities/repo/package-artifact-rebuild.tspackages/worker/src/mcp/capabilities/repo/repo-publish-session.tspackages/worker/src/mcp/capabilities/repo/repo-run-commands.tspackages/worker/src/mcp/capabilities/repo/repo-workflow.node.test.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-runtime/published-bundle-artifacts.node.test.tspackages/worker/src/package-runtime/published-bundle-artifacts.tspackages/worker/src/repo/external-publish.tspackages/worker/src/repo/repo-session-do.tspackages/worker/src/repo/repo-session-rpc.ts
💤 Files with no reviewable changes (1)
- packages/worker/src/package-runtime/published-bundle-artifacts.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/worker/src/mcp/capabilities/repo/repo-publish-session.ts
- packages/worker/src/repo/repo-session-rpc.ts
- packages/worker/src/package-runtime/published-bundle-artifacts.ts
- packages/worker/src/mcp/capabilities/packages/publish-external-push.ts
- packages/worker/src/mcp/capabilities/repo/repo-run-commands.ts
- packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts
- packages/worker/src/repo/repo-session-do.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
4 of 5 tasks
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause
Large saved package publishes were doing too much work inside one
RepoSessionDurable Object invocation. The hot path ran checks, kept fullsourceFilessnapshots reachable, wrote whole-source KV snapshots, refreshed package projection, and rebuilt every published bundle artifact in the same isolate.repo_run_commandsalso returned the fullsourceFilesmap fromrunRepoChecksthrough the DO RPC object before MCP output normalization discarded it. For large/multi-entrypoint packages such asai-chatandemail-received-subscriber, that stacked whole source trees, typecheck/bundle state, serialized snapshots, and bundle artifact module maps in one memory lifetime.Memory hotspots found
runRepoCheckscollects a fullRecord<string, string>source tree and builds a second worker-bundler snapshot from it.RepoSession.runCheckswas spreading that full check result back to callers, sosourceFilescould be serialized over RPC even though public schemas do not expose it.publishSessioncollected the full workspace again after checks.finalizePublishedEntitySourceadvanced source metadata and then calledrefreshSavedPackageProjection, which loaded source again and rebuilt every package bundle artifact in the same DO publish invocation.already_publishedwithout repairing stale/missing bundle artifacts.Chosen design
sourceFilesbefore persisting/check-returningRepoSessioncheck results, so large source trees are not serialized to MCP callers.collectPublishedPackageArtifactTargets) and single-target persistence (persistPublishedPackageArtifactTarget).rebuildArtifacts: false.repo_run_commands(... publish: true)andpackage_publish_external_push) publish/finalize source metadata first, then rebuild saved-package artifacts one target at a time through separateRepoSessionRPC invocations.package_publish_external_push, remove the earlyalready_publishedshortcut: the capability now clones/prepares the workspace and rebuilds artifacts even when D1 already points at the current Artifacts HEAD. This gives operators a safe repair path for stale/missing bundles.Tests run
npx vitest run --project node-unit packages/worker/src/mcp/capabilities/packages/publish-external-push.node.test.ts packages/worker/src/mcp/capabilities/repo/repo-workflow.node.test.ts packages/worker/src/repo/repo-session-do.node.test.ts packages/worker/src/repo/external-publish.node.test.ts && npm run typechecknpm run validate(format, lint, typecheck, unit tests, Playwright E2E, MCP E2E all completed successfully)Operational steps after deploy
Re-run
package_publish_external_pushfor the blocked packages, starting withemail-received-subscriber(2710d748-686c-4cfa-8425-6b34f7d204a3) andai-chat. It is now useful even if Artifacts HEAD already equalspublished_commit, because the already-published path rebuilds current bundle artifacts one target at a time. Then republish affected dependents (discord-gateway,discord-general-chat,social-intelligence, and other stale static dependents reported by the response) as needed so their statickody:@snapshots capture the refreshed dependency commits.Summary by CodeRabbit
New Features
Refactor
Tests