Skip to content

fix: clear relation listeners to prevent leaks #356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kettanaito merged 1 commit into from
Apr 19, 2026
Merged

fix: clear relation listeners to prevent leaks #356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/extensions/sync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ export function sync() {
logger.log('updating record...')

await performWithoutBroadcasting(async () => {
collection.update(
await collection.update(
(q: Query<any>) =>
q.where((record: RecordType) => {
return record[kPrimaryKey] === data.primaryKey
Expand Down
305 changes: 172 additions & 133 deletions src/relation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,47 +141,75 @@ export abstract class Relation {
// This way, each record has its own instance of a stateful relation.
this.#initializeRelation(path, record, initialValues)

/**
* @note Tear down all hook listeners registered below once the owner
* record is deleted. Aborting the signal removes the listeners from
* the underlying emitter — including the `delete` listener itself.
*/
const abortController = new AbortController()

this.ownerCollection.hooks.on(
'delete',
(event) => {
if (
event.data.deletedRecord[kRelationMap].get(serializedPath) === this
) {
abortController.abort()
}
},
{ signal: abortController.signal },
)

for (const foreignCollection of this.foreignCollections) {
// Update the owner relations when a foreign record is created
// referencing the owner record.
foreignCollection.hooks.on('create', (event) => {
const { record: foreignRecord } = event.data
const foreignRelations = this.getRelationsToOwner(foreignRecord)

for (const foreignRelation of foreignRelations) {
const ownerRecords = this.ownerCollection.findMany((q) =>
q.where((record) => {
return foreignRelation.foreignKeys.has(record[kPrimaryKey])
}),
)
foreignCollection.hooks.on(
'create',
(event) => {
const { record: foreignRecord } = event.data
const foreignRelations = this.getRelationsToOwner(foreignRecord)

for (const foreignRelation of foreignRelations) {
const ownerRecords = this.ownerCollection.findMany((q) =>
q.where((record) => {
return foreignRelation.foreignKeys.has(record[kPrimaryKey])
}),
)

for (const ownerRecord of ownerRecords) {
const ownerRelation = ownerRecord[kRelationMap].get(serializedPath)
ownerRelation.foreignKeys.add(foreignRecord[kPrimaryKey])
for (const ownerRecord of ownerRecords) {
const ownerRelation =
ownerRecord[kRelationMap].get(serializedPath)
ownerRelation.foreignKeys.add(foreignRecord[kPrimaryKey])
}
}
}
})
},
{ signal: abortController.signal },
)

// Clear the references to deleted foreign records.
foreignCollection.hooks.on('delete', (event) => {
const { deletedRecord: deletedForeignRecord } = event.data
this.foreignKeys.delete(deletedForeignRecord[kPrimaryKey])

// Delete all the owners referencing the deleted foreign record
// if the relation is set to cascade on delete.
if (this.options.onDelete === 'cascade') {
const foreignRelations =
this.getRelationsToOwner(deletedForeignRecord)

this.ownerCollection.deleteMany((q) => {
return q.where((record) => {
return foreignRelations.some((foreignRelation) => {
return foreignRelation.foreignKeys.has(record[kPrimaryKey])
foreignCollection.hooks.on(
'delete',
(event) => {
const { deletedRecord: deletedForeignRecord } = event.data
this.foreignKeys.delete(deletedForeignRecord[kPrimaryKey])

// Delete all the owners referencing the deleted foreign record
// if the relation is set to cascade on delete.
if (this.options.onDelete === 'cascade') {
const foreignRelations =
this.getRelationsToOwner(deletedForeignRecord)

this.ownerCollection.deleteMany((q) => {
return q.where((record) => {
return foreignRelations.some((foreignRelation) => {
return foreignRelation.foreignKeys.has(record[kPrimaryKey])
})
})
})
})
}
})
}
},
{ signal: abortController.signal },
)
}

/**
Expand All @@ -192,126 +220,137 @@ export abstract class Relation {
* @example
* await users.update(q, { data: { country: { code: 'uk' } } })
*/
this.ownerCollection.hooks.earlyOn('update', (event) => {
const update = event.data

if (
path.every((key, index) => key === update.path[index]) &&
!isRecord(update.nextValue)
) {
/**
* @note Listeners are attached per-record but fire for every owner update.
* Skip events whose target record's relation isn't this instance.
*/
if (update.prevRecord[kRelationMap].get(serializedPath) !== this) {
return
}
this.ownerCollection.hooks.earlyOn(
'update',
(event) => {
const update = event.data

if (
path.every((key, index) => key === update.path[index]) &&
!isRecord(update.nextValue)
) {
/**
* @note Listeners are attached per-record but fire for every owner update.
* Skip events whose target record's relation isn't this instance.
*/
if (update.prevRecord[kRelationMap].get(serializedPath) !== this) {
return
}

event.preventDefault()
event.stopImmediatePropagation()
event.preventDefault()
event.stopImmediatePropagation()

const foreignUpdatePath = update.path.slice(path.length)
const foreignUpdatePath = update.path.slice(path.length)

for (const foreignCollection of this.foreignCollections) {
foreignCollection.updateMany(
(q) => {
return q.where((record) => {
return this.foreignKeys.has(record[kPrimaryKey])
})
},
{
data(foreignRecord) {
set(foreignRecord, foreignUpdatePath, update.nextValue)
for (const foreignCollection of this.foreignCollections) {
foreignCollection.updateMany(
(q) => {
return q.where((record) => {
return this.foreignKeys.has(record[kPrimaryKey])
})
},
},
)
{
data(foreignRecord) {
set(foreignRecord, foreignUpdatePath, update.nextValue)
},
},
)
}
}
}
})
},
{ signal: abortController.signal },
)

/**
* Handle owner updates where the relational property changes to another foreign record.
*
* @example
* await users.update(q, { data: { country: await countries.create({}) } })
*/
this.ownerCollection.hooks.on('update', (event) => {
const update = event.data

if (isEqual(update.path, path) && isRecord(update.nextValue)) {
/**
* @note Listeners are attached per-record but fire for every owner update.
* Skip events whose target record's relation isn't this instance.
*/
if (update.prevRecord[kRelationMap].get(serializedPath) !== this) {
return
}

event.preventDefault()

// If the owner relation is "one-of", multiple foreign records cannot own this record.
// Disassociate the old foreign records from pointing to the owner record.
if (this instanceof One) {
const oldForeignRecords = this.foreignCollections.flatMap<RecordType>(
(foreignCollection) => {
return foreignCollection.findMany((q) => {
return q.where((record) => {
return this.foreignKeys.has(record[kPrimaryKey])
})
})
},
)

const foreignRelationsToDisassociate = oldForeignRecords.flatMap(
(record) => this.getRelationsToOwner(record),
)

// Throw if attempting to disassociate unique relations.
if (this.options.unique) {
invariant.as(
RelationError.for(
RelationErrorCodes.FORBIDDEN_UNIQUE_UPDATE,
this.#createErrorDetails(),
),
foreignRelationsToDisassociate.length === 0,
'Failed to update a unique relation at "%s": the foreign record is already associated with another owner',
update.path.join('.'),
)
}

for (const foreignRelation of foreignRelationsToDisassociate) {
foreignRelation.foreignKeys.delete(update.prevRecord[kPrimaryKey])
this.ownerCollection.hooks.on(
'update',
(event) => {
const update = event.data

if (isEqual(update.path, path) && isRecord(update.nextValue)) {
/**
* @note Listeners are attached per-record but fire for every owner update.
* Skip events whose target record's relation isn't this instance.
*/
if (update.prevRecord[kRelationMap].get(serializedPath) !== this) {
return
}

// Check any other owners associated with the same foreign record.
// This is important since unique relations are not always two-way.
if (this.options.unique) {
const otherOwnersAssociatedWithForeignRecord =
this.#getOtherOwnerForRecords([update.nextValue])

invariant.as(
RelationError.for(
RelationErrorCodes.FORBIDDEN_UNIQUE_UPDATE,
this.#createErrorDetails(),
),
otherOwnersAssociatedWithForeignRecord == null,
'Failed to update a unique relation at "%s": the foreign record is already associated with another owner',
update.path.join('.'),
event.preventDefault()

// If the owner relation is "one-of", multiple foreign records cannot own this record.
// Disassociate the old foreign records from pointing to the owner record.
if (this instanceof One) {
const oldForeignRecords =
this.foreignCollections.flatMap<RecordType>(
(foreignCollection) => {
return foreignCollection.findMany((q) => {
return q.where((record) => {
return this.foreignKeys.has(record[kPrimaryKey])
})
})
},
)

const foreignRelationsToDisassociate = oldForeignRecords.flatMap(
(record) => this.getRelationsToOwner(record),
)
}

this.foreignKeys.clear()
}
// Throw if attempting to disassociate unique relations.
if (this.options.unique) {
invariant.as(
RelationError.for(
RelationErrorCodes.FORBIDDEN_UNIQUE_UPDATE,
this.#createErrorDetails(),
),
foreignRelationsToDisassociate.length === 0,
'Failed to update a unique relation at "%s": the foreign record is already associated with another owner',
update.path.join('.'),
)
}

for (const foreignRelation of foreignRelationsToDisassociate) {
foreignRelation.foreignKeys.delete(update.prevRecord[kPrimaryKey])
}

// Check any other owners associated with the same foreign record.
// This is important since unique relations are not always two-way.
if (this.options.unique) {
const otherOwnersAssociatedWithForeignRecord =
this.#getOtherOwnerForRecords([update.nextValue])

invariant.as(
RelationError.for(
RelationErrorCodes.FORBIDDEN_UNIQUE_UPDATE,
this.#createErrorDetails(),
),
otherOwnersAssociatedWithForeignRecord == null,
'Failed to update a unique relation at "%s": the foreign record is already associated with another owner',
update.path.join('.'),
)
}

this.foreignKeys.clear()
}

// Associate the owner with a foreign record from the update data.
const foreignRecord = update.nextValue
this.foreignKeys.add(foreignRecord[kPrimaryKey])
// Associate the owner with a foreign record from the update data.
const foreignRecord = update.nextValue
this.foreignKeys.add(foreignRecord[kPrimaryKey])

for (const foreignRelation of this.getRelationsToOwner(foreignRecord)) {
foreignRelation.foreignKeys.add(update.prevRecord[kPrimaryKey])
for (const foreignRelation of this.getRelationsToOwner(
foreignRecord,
)) {
foreignRelation.foreignKeys.add(update.prevRecord[kPrimaryKey])
}
}
}
})
},
{ signal: abortController.signal },
)
}

public abstract resolve(foreignKeys: Set<string>): unknown
Expand Down
34 changes: 34 additions & 0 deletions tests/relations/one-to-one.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,3 +617,37 @@ it('scopes a nested one-to-one relation update to the targeted record', async ()

expect(countries.all()).toEqual([{ code: 'uk' }, { code: 'ca' }])
})

it('removes relation listeners when the owner record is deleted', async () => {
const users = new Collection({ schema: userSchema })
const countries = new Collection({ schema: countrySchema })

users.defineRelations(({ one }) => ({
country: one(countries),
}))

const totalListeners = () =>
countries.hooks.listenerCount('create') +
countries.hooks.listenerCount('delete') +
users.hooks.listenerCount('update') +
users.hooks.listenerCount('delete')

const baseline = totalListeners()

const user = await users.create({
id: 1,
country: await countries.create({ code: 'us' }),
})

expect(
totalListeners(),
'Attaches relation listeners when the owner record is created',
).toBeGreaterThan(baseline)

users.delete(user)

expect(
totalListeners(),
'Detaches relation listeners when the owner record is deleted',
).toBe(baseline)
})
Loading