Skip to content

Commit 670e47b

Browse files
authored
@tus/file-store: preserve completed uploads during expiration cleanup (#898)
related to #320 (introduced) related to #894 (extended) Currently, metadata offset is checked but it's never persisted. Cleanup could remove completed files. Compare the actual file size instead. There is an inherent issue for uploads in progress because cleanup doesn't take the lock but it should be handled separately since requires a refactoring. <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/tus/tus-node-server/pull/898" target="_blank"><picture><source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-devin-review-dark.svg?v=4"><img src="https://static.devin.ai/assets/gh-devin-review-light.svg?v=4" alt="Devin Review"></picture></a> <!-- devin-review-badge-end --> Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
1 parent a7bfaa4 commit 670e47b

4 files changed

Lines changed: 132 additions & 20 deletions

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@tus/file-store": patch
3+
---
4+
5+
Preserve completed uploads during expiration cleanup by checking the actual file size instead of the persisted metadata offset. Skip uploads that disappear during cleanup.

‎packages/file-store/src/index.ts‎

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -177,39 +177,56 @@ export class FileStore extends DataStore {
177177
}
178178

179179
async deleteExpired(): Promise<number> {
180-
const now = new Date()
181-
const toDelete: Promise<void>[] = []
182-
183180
if (!this.configstore.list) {
184181
throw ERRORS.UNSUPPORTED_EXPIRATION_EXTENSION
185182
}
183+
const expiration = this.getExpiration()
184+
if (expiration <= 0) {
185+
return 0
186+
}
186187

188+
const now = Date.now()
189+
const toDelete: Promise<void>[] = []
190+
let deleted = 0
187191
const uploadKeys = await this.configstore.list()
188192
for (const file_id of uploadKeys) {
189193
try {
190-
const info = await this.configstore.get(file_id)
194+
const info = await this.getUpload(file_id)
191195
if (
192-
info &&
193-
'creation_date' in info &&
194-
this.getExpiration() > 0 &&
195196
info.size !== info.offset &&
196-
info.creation_date
197+
info.creation_date &&
198+
now > Date.parse(info.creation_date) + expiration
197199
) {
198-
const creation = new Date(info.creation_date)
199-
const expires = new Date(creation.getTime() + this.getExpiration())
200-
if (now > expires) {
201-
toDelete.push(this.remove(file_id))
202-
}
200+
toDelete.push(
201+
this.remove(file_id).then(
202+
() => {
203+
deleted++
204+
},
205+
async (error) => {
206+
if (error !== ERRORS.FILE_NOT_FOUND) throw error
207+
208+
try {
209+
await fsProm.lstat(this.resolve(file_id))
210+
} catch (statError) {
211+
if ((statError as NodeJS.ErrnoException).code === FILE_DOESNT_EXIST) {
212+
return
213+
}
214+
throw statError
215+
}
216+
throw error
217+
}
218+
)
219+
)
203220
}
204221
} catch (error) {
205-
if (error !== ERRORS.FILE_NO_LONGER_EXISTS) {
222+
if (error !== ERRORS.FILE_NOT_FOUND && error !== ERRORS.FILE_NO_LONGER_EXISTS) {
206223
throw error
207224
}
208225
}
209226
}
210227

211228
await Promise.all(toDelete)
212-
return toDelete.length
229+
return deleted
213230
}
214231

215232
getExpiration(): number {

‎packages/file-store/src/test/index.ts‎

Lines changed: 93 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,8 +40,7 @@ describe('FileStore', function () {
4040
})
4141

4242
this.afterEach(async () => {
43-
// @ts-expect-error ignore
44-
fs.mkdir.restore()
43+
sinon.restore()
4544
await cleanup()
4645
})
4746

@@ -132,8 +131,44 @@ describe('FileStore', function () {
132131
})
133132

134133
describe('deleteExpired', () => {
135-
it('removes expired nested uploads and keeps unexpired uploads', async function () {
134+
beforeEach(function () {
136135
this.datastore.expirationPeriodInMilliseconds = 60_000
136+
})
137+
138+
const expiredUpload = (id: string, size = 10) =>
139+
new Upload({
140+
id,
141+
size,
142+
offset: 0,
143+
creation_date: new Date(Date.now() - 120_000).toISOString(),
144+
})
145+
146+
it('skips listing when expiration is 0', async function () {
147+
this.datastore.expirationPeriodInMilliseconds = 0
148+
const list = sinon.stub(this.datastore.configstore, 'list').resolves([])
149+
150+
assert.equal(await this.datastore.deleteExpired(), 0)
151+
assert.equal(list.called, false)
152+
})
153+
154+
for (const [id, contents] of [
155+
['completed', Buffer.from('completed upload')],
156+
['nested/path/completed', Buffer.from('completed upload')],
157+
['empty', Buffer.alloc(0)],
158+
] as const) {
159+
it(`keeps completed upload ${id} after expiration`, async function () {
160+
await this.datastore.create(expiredUpload(id, contents.length))
161+
if (contents.length) {
162+
await this.datastore.write(Readable.from(contents), id, 0)
163+
}
164+
165+
assert.equal(await this.datastore.deleteExpired(), 0)
166+
assert.equal((await this.datastore.getUpload(id)).offset, contents.length)
167+
assert.deepEqual(await fsProm.readFile(path.join(storePath, id)), contents)
168+
})
169+
}
170+
171+
it('removes expired nested uploads and keeps unexpired uploads', async function () {
137172
const expired = new Upload({
138173
id: 'nested/path/expired',
139174
size: 10,
@@ -153,6 +188,61 @@ describe('FileStore', function () {
153188
})
154189
assert.equal((await this.datastore.getUpload(fresh.id)).id, fresh.id)
155190
})
191+
192+
for (const suffix of ['', '.json']) {
193+
it(`continues cleanup when missing${suffix} disappears after listing`, async function () {
194+
for (const id of ['missing', 'remaining']) {
195+
await this.datastore.create(expiredUpload(id))
196+
}
197+
const store = this.datastore.configstore
198+
const list = store.list.bind(store)
199+
sinon.stub(store, 'list').callsFake(async () => {
200+
const ids = await list()
201+
await fsProm.rm(path.join(storePath, `missing${suffix}`))
202+
return ids
203+
})
204+
205+
assert.equal(await this.datastore.deleteExpired(), 1)
206+
await assert.rejects(fsProm.stat(path.join(storePath, 'remaining')), {
207+
code: 'ENOENT',
208+
})
209+
})
210+
}
211+
212+
it('continues cleanup when an upload disappears before removal', async function () {
213+
for (const id of ['missing', 'remaining']) {
214+
await this.datastore.create(expiredUpload(id))
215+
}
216+
const remove = this.datastore.remove.bind(this.datastore)
217+
sinon.stub(this.datastore, 'remove').callsFake(async (id: unknown) => {
218+
if (id === 'missing') {
219+
await fsProm.rm(path.join(storePath, id))
220+
}
221+
return remove(id as string)
222+
})
223+
224+
assert.equal(await this.datastore.deleteExpired(), 1)
225+
await assert.rejects(fsProm.stat(path.join(storePath, 'remaining')), {
226+
code: 'ENOENT',
227+
})
228+
})
229+
230+
for (const lstatFails of [false, true]) {
231+
it(`rejects when removal fails and ${lstatFails ? 'the file check fails' : 'the file remains'}`, async function () {
232+
await this.datastore.create(expiredUpload('locked'))
233+
const error = Object.assign(new Error('EIO'), {code: 'EIO'})
234+
sinon.stub(fsProm, 'unlink').rejects(error)
235+
if (lstatFails) {
236+
sinon.stub(fsProm, 'lstat').rejects(error)
237+
}
238+
239+
await assert.rejects(
240+
this.datastore.deleteExpired(),
241+
lstatFails ? (thrown) => thrown === error : {status_code: 404}
242+
)
243+
await fsProm.stat(path.join(storePath, 'locked'))
244+
})
245+
}
156246
})
157247

158248
describe('FileConfigstore', () => {

‎packages/utils/src/test/stores.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,10 +92,10 @@ export const shouldExpireUploads = () => {
9292
assert.equal(this.datastore.hasExtension('expiration'), true)
9393
})
9494

95-
it('should expire upload', async function () {
95+
it('should expire incomplete upload', async function () {
9696
const file = new Upload({
9797
id: testId('expiration-test'),
98-
size: this.testFileSize,
98+
size: this.testFileSize + 1,
9999
offset: 0,
100100
metadata: {filename: 'world_domination_plan.pdf', is_confidential: null},
101101
})

0 commit comments

Comments
 (0)