Skip to content

fix(file): use the sync xattr API, the async path of fs-xattr leaks #1357

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
ferhatelmas merged 2 commits into from
Sep 2, 2026
Merged

fix(file): use the sync xattr API, the async path of fs-xattr leaks #1357

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9c91c8
fix(file): use the sync xattr API, the async path of fs-xattr leaks
guillaume-flambard Sep 2, 2026
4c51b85
fix: handle async context
ferhatelmas Sep 2, 2026
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
68 changes: 38 additions & 30 deletions src/storage/backend/file.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ import { withOptionalVersion } from './adapter'
import { FileBackend } from './file'

vi.mock('fs-xattr', () => ({
setAttribute: vi.fn(() => Promise.resolve()),
getAttribute: vi.fn(() => Promise.resolve(undefined)),
removeAttribute: vi.fn(() => Promise.resolve()),
setAttributeSync: vi.fn(() => undefined),
getAttributeSync: vi.fn(() => undefined),
removeAttributeSync: vi.fn(() => undefined),
}))

describe('FileBackend xattr metadata', () => {
Expand Down Expand Up @@ -48,7 +48,7 @@ describe('FileBackend xattr metadata', () => {

await backend.uploadPart('bucket', 'key', 'v1', uploadId as string, 1, Readable.from('hello'))

expect(xattr.setAttribute).toHaveBeenCalledWith(
expect(xattr.setAttributeSync).toHaveBeenCalledWith(
expect.any(String),
'user.supabase.etag',
expect.any(String)
Expand Down Expand Up @@ -107,12 +107,12 @@ describe('FileBackend xattr metadata', () => {
await fsp.mkdir(partDir, { recursive: true })
await fsp.writeFile(partPath, 'hello')

const xattrGet = xattr.getAttribute as unknown as Mock
const xattrGet = xattr.getAttributeSync as unknown as Mock
xattrGet.mockImplementation((_file: string, attribute: string) => {
if (attribute === 'user.supabase.etag') {
return Promise.resolve(Buffer.from('part-etag'))
return Buffer.from('part-etag')
}
return Promise.resolve(undefined)
return undefined
})

uploadSpy = vi
Expand Down Expand Up @@ -142,7 +142,7 @@ describe('FileBackend xattr metadata', () => {
ETag: '"final"',
})

expect(xattr.getAttribute).toHaveBeenCalledWith(expect.any(String), 'user.supabase.etag')
expect(xattr.getAttributeSync).toHaveBeenCalledWith(expect.any(String), 'user.supabase.etag')
} finally {
uploadSpy?.mockRestore()
if (originalPlatformDescriptor) {
Expand Down Expand Up @@ -419,19 +419,19 @@ describe('FileBackend copy metadata options', () => {
getConfig({ reload: true })
backend = new FileBackend()

const xattrGet = xattr.getAttribute as unknown as Mock
const xattrGet = xattr.getAttributeSync as unknown as Mock
xattrGet.mockReset()
xattrGet.mockImplementation((_file: string, attribute: string) => {
if (attribute === 'user.supabase.cache-control') {
return Promise.resolve(Buffer.from('max-age=60'))
return Buffer.from('max-age=60')
}
if (attribute === 'user.supabase.content-type') {
return Promise.resolve(Buffer.from('text/plain'))
return Buffer.from('text/plain')
}
return Promise.resolve(undefined)
return undefined
})
;(xattr.setAttribute as unknown as Mock).mockReset().mockResolvedValue(undefined)
;(xattr.removeAttribute as unknown as Mock).mockReset().mockResolvedValue(undefined)
;(xattr.setAttributeSync as unknown as Mock).mockReset()
;(xattr.removeAttributeSync as unknown as Mock).mockReset()

await backend.uploadObject(
'bucket',
Expand All @@ -441,14 +441,14 @@ describe('FileBackend copy metadata options', () => {
'text/plain',
'max-age=60'
)
;(xattr.setAttribute as unknown as Mock).mockClear()
;(xattr.removeAttribute as unknown as Mock).mockClear()
;(xattr.setAttributeSync as unknown as Mock).mockClear()
;(xattr.removeAttributeSync as unknown as Mock).mockClear()
})

afterEach(async () => {
;(xattr.getAttribute as unknown as Mock).mockReset().mockResolvedValue(undefined)
;(xattr.setAttribute as unknown as Mock).mockReset().mockResolvedValue(undefined)
;(xattr.removeAttribute as unknown as Mock).mockReset().mockResolvedValue(undefined)
;(xattr.getAttributeSync as unknown as Mock).mockReset()
;(xattr.setAttributeSync as unknown as Mock).mockReset()
;(xattr.removeAttributeSync as unknown as Mock).mockReset()
if (originalPlatformDescriptor) {
Object.defineProperty(process, 'platform', originalPlatformDescriptor)
}
Expand Down Expand Up @@ -531,12 +531,12 @@ describe('FileBackend copy metadata options', () => {
cacheControl: 'max-age=999',
contentType: undefined,
})
expect(xattr.setAttribute).toHaveBeenCalledWith(
expect(xattr.setAttributeSync).toHaveBeenCalledWith(
expect.any(String),
'user.supabase.cache-control',
'max-age=999'
)
expect(xattr.removeAttribute).toHaveBeenCalledWith(
expect(xattr.removeAttributeSync).toHaveBeenCalledWith(
expect.any(String),
'user.supabase.content-type'
)
Expand All @@ -554,21 +554,23 @@ describe('FileBackend copy metadata options', () => {
{ copyMetadata: false }
)

expect(xattr.setAttribute).not.toHaveBeenCalled()
expect(xattr.removeAttribute).toHaveBeenCalledTimes(2)
expect(xattr.removeAttribute).toHaveBeenCalledWith(
expect(xattr.setAttributeSync).not.toHaveBeenCalled()
expect(xattr.removeAttributeSync).toHaveBeenCalledTimes(2)
expect(xattr.removeAttributeSync).toHaveBeenCalledWith(
expect.any(String),
'user.supabase.cache-control'
)
expect(xattr.removeAttribute).toHaveBeenCalledWith(
expect(xattr.removeAttributeSync).toHaveBeenCalledWith(
expect.any(String),
'user.supabase.content-type'
)
})

it('preserves absent source metadata when copyMetadata is true', async () => {
const missingXattr = Object.assign(new Error('missing xattr'), { code: 'ENODATA' })
;(xattr.getAttribute as unknown as Mock).mockRejectedValue(missingXattr)
;(xattr.getAttributeSync as unknown as Mock).mockImplementation(() => {
throw missingXattr
})

await expect(
backend.copyObject(
Expand All @@ -583,12 +585,14 @@ describe('FileBackend copy metadata options', () => {
)
).resolves.toMatchObject({ httpStatusCode: 200 })

expect(xattr.removeAttribute).toHaveBeenCalledTimes(2)
expect(xattr.removeAttributeSync).toHaveBeenCalledTimes(2)
})

it('ignores already absent destination metadata', async () => {
const missingXattr = Object.assign(new Error('missing xattr'), { code: 'ENOATTR' })
;(xattr.removeAttribute as unknown as Mock).mockRejectedValue(missingXattr)
;(xattr.removeAttributeSync as unknown as Mock).mockImplementation(() => {
throw missingXattr
})

await expect(
backend.copyObject(
Expand All @@ -606,7 +610,9 @@ describe('FileBackend copy metadata options', () => {

it('propagates genuine source metadata read errors', async () => {
const readError = Object.assign(new Error('xattr read failed'), { code: 'EIO' })
;(xattr.getAttribute as unknown as Mock).mockRejectedValue(readError)
;(xattr.getAttributeSync as unknown as Mock).mockImplementation(() => {
throw readError
})

await expect(
backend.copyObject(
Expand All @@ -624,7 +630,9 @@ describe('FileBackend copy metadata options', () => {

it('propagates genuine destination metadata removal errors', async () => {
const removeError = Object.assign(new Error('xattr removal failed'), { code: 'EIO' })
;(xattr.removeAttribute as unknown as Mock).mockRejectedValue(removeError)
;(xattr.removeAttributeSync as unknown as Mock).mockImplementation(() => {
throw removeError
})

await expect(
backend.copyObject(
Expand Down
12 changes: 8 additions & 4 deletions src/storage/backend/file.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,10 @@ export class FileBackend implements StorageBackendAdapter {

protected async getMetadataAttr(file: string, attribute: string): Promise<string | undefined> {
try {
const value = await xattr.getAttribute(file, attribute)
// fs-xattr's async path leaks about 0.5 kB per call (fs-xattr#47, reported
// for this backend in #1349). The sync path does not leak and the reads
// are small, so it is used until an upstream release ships the fix.
const value = xattr.getAttributeSync(file, attribute)
return value?.toString() ?? undefined
} catch (error) {
if (isMissingXattrError(error)) {
Expand All @@ -618,8 +621,9 @@ export class FileBackend implements StorageBackendAdapter {
}
}

protected setMetadataAttr(file: string, attribute: string, value: string): Promise<void> {
return xattr.setAttribute(file, attribute, value)
protected async setMetadataAttr(file: string, attribute: string, value: string): Promise<void> {
// see getMetadataAttr: sync xattr calls until fs-xattr ships a fixed release
xattr.setAttributeSync(file, attribute, value)
}

protected async setOrRemoveMetadataAttr(
Expand All @@ -633,7 +637,7 @@ export class FileBackend implements StorageBackendAdapter {
}

try {
await xattr.removeAttribute(file, attribute)
xattr.removeAttributeSync(file, attribute)
} catch (error) {
if (!isMissingXattrError(error)) {
throw error
Expand Down