fix: stop object reference leak in AppFabricationFulfillment#36943
fix: stop object reference leak in AppFabricationFulfillment#36943kodiakhq[bot] merged 9 commits intodevelopfrom
Conversation
🦋 Changeset detectedLatest commit: 59684fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 packages
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 |
|
Looks like this PR is ready to merge! 🎉 |
WalkthroughReplaced internal-state assignments with deep copies via structuredClone in AppFabricationFulfillment setters, added unit tests validating deep-clone semantics (including empty-object handling), and added a changeset entry; no public API or signature changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant AFF as AppFabricationFulfillment
participant Clone as structuredClone
Caller->>AFF: setAppInfo(info)
AFF->>Clone: structuredClone(info)
Clone-->>AFF: deepCopy
AFF-->>Caller: stored deep copy
Caller->>AFF: getAppInfo()
AFF-->>Caller: stored deep copy
Caller->>AFF: setImplementedInterfaces(map)
AFF->>Clone: structuredClone(map)
Clone-->>AFF: deepCopy
AFF-->>Caller: stored deep copy
Caller->>AFF: getImplementedInterfaces()
AFF-->>Caller: stored deep copy
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Comment |
![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 (3)
packages/apps-engine/src/server/compiler/AppFabricationFulfillment.ts (1)
41-42: Shallow copy is sufficient for a boolean map; structuredClone is overkill here.This object is one level deep with primitive values. A spread copy avoids the heavier deep‑clone and is clearer.
- this.implemented = structuredClone(interfaces); + this.implemented = { ...interfaces };Side note: method name
getImplementedInferfaces()contains a typo. Consider adding a correctly spelled alias (getImplementedInterfaces) and deprecating the old one in a follow‑up to avoid a breaking change.packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (2)
56-99: Strengthen the test by asserting reference inequality (true deep clone).Add identity checks so the test fails if a shallow copy slips in.
const aff = new AppFabricationFulfillment(); aff.setAppInfo(originalInfo); // Verify the stored info is equal to the original Expect(aff.getAppInfo()).toEqual(originalInfo); + // And not the same references (deep clone) + Expect(aff.getAppInfo()).not.toBe(originalInfo); + Expect(aff.getAppInfo().author).not.toBe(originalInfo.author); + Expect(aff.getAppInfo().implements).not.toBe(originalInfo.implements); + Expect(aff.getAppInfo().permissions).not.toBe(originalInfo.permissions);
100-127: Add an identity check for the interfaces map; consider naming fix follow‑up.Validate it’s not the same reference. Also consider adding a new
getImplementedInterfaces()alias and deprecating the misspelled one.// Verify the stored interfaces are equal to the original Expect(aff.getImplementedInferfaces()).toEqual(originalInterfaces); + // And not the same reference + Expect(aff.getImplementedInferfaces()).not.toBe(originalInterfaces);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/apps-engine/src/server/compiler/AppFabricationFulfillment.ts(2 hunks)packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (2)
packages/apps-engine/src/server/permissions/AppPermissions.ts (1)
AppPermissions(24-125)packages/apps-engine/src/server/compiler/AppFabricationFulfillment.ts (1)
AppFabricationFulfillment(5-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (2)
11-11: LGTM: importing AppPermissions improves test realism.Keeps the permissions payload representative of production data.
129-137: LGTM: empty object handling + non‑identity check.Covers the edge case well; assertions are appropriate.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36943 +/- ##
===========================================
- Coverage 67.33% 66.28% -1.05%
===========================================
Files 3339 3393 +54
Lines 113205 115451 +2246
Branches 20535 21165 +630
===========================================
+ Hits 76224 76525 +301
- Misses 34376 36312 +1936
- Partials 2605 2614 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
force-pushed
the
fix/app-fabrication-fulfillment-leak
branch
from
2a03f7b to
9dfb018
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (2)
56-98: Strengthen deep‑clone assertions (identity + size checks).Add reference and size assertions to make the clone guarantees explicit and avoid false positives if equality semantics change.
@@ const aff = new AppFabricationFulfillment(); aff.setAppInfo(originalInfo); + // Identity checks: internal copy must not alias the input + Expect(aff.getAppInfo()).not.toBe(originalInfo); + Expect(aff.getAppInfo().author).not.toBe(originalInfo.author); + Expect(aff.getAppInfo().implements).not.toBe(originalInfo.implements); + Expect(aff.getAppInfo().permissions).not.toBe(originalInfo.permissions); @@ originalInfo.implements.push(AppInterface.IPostMessageSent); originalInfo.permissions.push(AppPermissions.message.write); @@ Expect(aff.getAppInfo().permissions).not.toContain(AppPermissions.message.write); + + // Sanity: sizes unchanged despite external mutations + Expect(aff.getAppInfo().implements.length).toEqual(1); + Expect(aff.getAppInfo().permissions.length).toEqual(2);
100-127: Also assert map identity and cardinality for implemented interfaces.Add a non‑aliasing check and invariant on key count.
@@ const aff = new AppFabricationFulfillment(); aff.setImplementedInterfaces(originalInterfaces); + // Identity check: stored map must not alias the input + Expect(aff.getImplementedInferfaces()).not.toBe(originalInterfaces); @@ Expect(aff.getImplementedInferfaces()[AppInterface.IPreMessageSentModify]).not.toBeDefined(); + + // Sanity: key count unchanged + Expect(Object.keys(aff.getImplementedInferfaces()).length).toEqual(3);Minor API nit: method name typo.
Consider adding an alias
getImplementedInterfaces()(and deprecating the misspelledgetImplementedInferfaces()), then update tests in a follow‑up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.changeset/tasty-ravens-grow.md(1 hunks)packages/apps-engine/src/server/compiler/AppFabricationFulfillment.ts(2 hunks)packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/tasty-ravens-grow.md
- packages/apps-engine/src/server/compiler/AppFabricationFulfillment.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (2)
packages/apps-engine/src/server/permissions/AppPermissions.ts (1)
AppPermissions(24-125)packages/apps-engine/src/server/compiler/AppFabricationFulfillment.ts (1)
AppFabricationFulfillment(5-75)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/apps-engine/tests/server/compiler/AppFabricationFulfillment.spec.ts (2)
11-11: LGTM: importing AppPermissions for realistic fixtures.
129-137: LGTM: empty object handled and no aliasing.
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
kodiakhq
bot
removed
the
stat: ready to merge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
2 participants
Proposed changes (including videos or screenshots)
During app installation and update, the AppFabricationFulfillment object that the AppManager returns will actually leak a direct reference to an IAppInfo object that is used inside the apps-engine scope.
This means that when server code interacts with that object, it will modify the object that is stored in the database. Saving data outside of the schema in the database can cause signature mismatch.
Issue(s)
Steps to test or reproduce
Further comments
ARCH-1804
Summary by CodeRabbit