Skip to content

Commit 6def6ef

Browse files
committed
fix: address must-fix and should-fix issues for release
- Throw on null mailboxInfo in getMailboxStatus to prevent TypeError crash - Use append mode when resuming exports to preserve previously exported data - Wrap markInterruptedAsFailed loop in try-catch so one failure does not abandon remaining entries; also clean up exports interrupted during indexing - Add 30s timeout to stream finalization to prevent indefinite hang - Remove encodeURIComponent from Outlook batch URLs to fix OData query parsing - Fix test score calculation to match production algorithm (SHA-256, mod 1M)
1 parent 29da53f commit 6def6ef

File tree

5 files changed

+73
-57
lines changed

5 files changed

+73
-57
lines changed

lib/email-client/imap/mailbox.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ class Mailbox {
108108
usable: connectionClient.usable,
109109
idling: connectionClient.idling
110110
});
111+
throw new Error('IMAP mailbox state is not available');
111112
}
112113

113114
let status = {

lib/email-client/outlook-client.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1684,7 +1684,7 @@ class OutlookClient extends BaseClient {
16841684
const request = {
16851685
id: reqId,
16861686
method: 'GET',
1687-
url: `/v1.0/${this.oauth2UserPath}/messages/${emailId}?$select=${encodeURIComponent(selectFields)}&$expand=${encodeURIComponent(expandFields)}`
1687+
url: `/v1.0/${this.oauth2UserPath}/messages/${emailId}?$select=${selectFields}&$expand=${expandFields}`
16881688
};
16891689

16901690
// Add Prefer header for body content type if specified

lib/export.js

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -489,27 +489,31 @@ class Export {
489489
const activeExports = await redis.smembers(ACTIVE_EXPORTS_KEY);
490490

491491
for (const entry of activeExports) {
492-
// Find ':exp_' as separator since account IDs may contain colons
493-
const separatorIndex = entry.indexOf(':exp_');
494-
if (separatorIndex === -1) continue;
495-
const account = entry.substring(0, separatorIndex);
496-
const exportId = entry.substring(separatorIndex + 1);
497-
if (!account || !exportId) continue;
498-
499-
const data = await redis.hgetall(getExportKey(account, exportId));
500-
501-
if (data && (data.status === 'processing' || data.status === 'queued')) {
502-
const job = await exportQueue.getJob(exportId).catch(() => null);
503-
if (job) {
504-
await job.remove().catch(() => {});
505-
logger.info({ msg: 'Removed interrupted export job from queue', account, exportId });
506-
}
492+
try {
493+
// Find ':exp_' as separator since account IDs may contain colons
494+
const separatorIndex = entry.indexOf(':exp_');
495+
if (separatorIndex === -1) continue;
496+
const account = entry.substring(0, separatorIndex);
497+
const exportId = entry.substring(separatorIndex + 1);
498+
if (!account || !exportId) continue;
499+
500+
const data = await redis.hgetall(getExportKey(account, exportId));
501+
502+
if (data && (data.status === 'processing' || data.status === 'queued' || data.status === 'indexing')) {
503+
const job = await exportQueue.getJob(exportId).catch(() => null);
504+
if (job) {
505+
await job.remove().catch(() => {});
506+
logger.info({ msg: 'Removed interrupted export job from queue', account, exportId });
507+
}
507508

508-
await Export.fail(account, exportId, 'Export interrupted by application restart');
509+
await Export.fail(account, exportId, 'Export interrupted by application restart');
509510

510-
if (data.filePath) {
511-
await fs.promises.unlink(data.filePath).catch(() => {});
511+
if (data.filePath) {
512+
await fs.promises.unlink(data.filePath).catch(() => {});
513+
}
512514
}
515+
} catch (err) {
516+
logger.error({ msg: 'Failed to mark interrupted export as failed', entry, err });
513517
}
514518
}
515519
}

test/export-test.js

Lines changed: 35 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,9 @@ const assert = require('node:assert').strict;
55
const crypto = require('crypto');
66
const msgpack = require('msgpack5')();
77

8-
// Test pure functions and logic without loading the full export module
9-
// This avoids the Bugsnag/logger initialization issues
8+
// Test pure functions and logic without loading the full export module.
9+
// This avoids the Bugsnag/logger initialization issues.
10+
// Score calculation must match lib/export.js Export.queueMessage() exactly.
1011

1112
const EXPORT_ID_PREFIX = 'exp_';
1213

@@ -15,21 +16,19 @@ function generateExportId() {
1516
return EXPORT_ID_PREFIX + crypto.randomBytes(12).toString('hex');
1617
}
1718

18-
// Replicate the score calculation logic
19-
// Uses messageId hash for tiebreaker instead of UID to avoid collisions with large UIDs
20-
// Using factor of 1000 to stay within JavaScript safe integer range (< 2^53)
21-
function calculateScore(timestamp, messageId) {
19+
// Replicate the score calculation logic from lib/export.js Export.queueMessage()
20+
// Uses SHA-256 hash of composite key (folder:messageId:uid) for tiebreaker
21+
// Using factor of 1000000 with baseSeconds to stay within JavaScript safe integer range (< 2^53)
22+
function calculateScore(timestamp, messageId, folder, uid) {
2223
const baseTimestamp = timestamp instanceof Date ? timestamp.getTime() : Number(timestamp) || Date.now();
24+
const baseSeconds = Math.floor(baseTimestamp / 1000);
2325

24-
// Generate tiebreaker from messageId hash (0-999 range)
25-
let tiebreaker = 0;
26-
const id = messageId || '';
27-
for (let i = 0; i < id.length; i++) {
28-
tiebreaker = ((tiebreaker << 5) - tiebreaker + id.charCodeAt(i)) | 0;
29-
}
30-
tiebreaker = Math.abs(tiebreaker) % 1000;
26+
// Generate tiebreaker from SHA-256 hash of composite key (0-999999 range)
27+
const uniqueKey = `${folder || ''}:${messageId || ''}:${uid || ''}`;
28+
const hash = crypto.createHash('sha256').update(uniqueKey).digest();
29+
const tiebreaker = ((hash[0] << 16) | (hash[1] << 8) | hash[2]) % 1000000;
3130

32-
return baseTimestamp * 1000 + tiebreaker;
31+
return baseSeconds * 1000000 + tiebreaker;
3332
}
3433

3534
// Replicate the formatStatus function logic
@@ -96,13 +95,14 @@ test('Export functionality tests', async t => {
9695
assert.ok(/^[0-9a-f]+$/.test(hexPart), 'Hex part should only contain hex characters');
9796
});
9897

99-
// Score calculation tests - using messageId hash for tiebreaker
98+
// Score calculation tests - using SHA-256 hash of composite key for tiebreaker
99+
// Production algorithm: lib/export.js Export.queueMessage()
100100
await t.test('Score calculation: different messageIds with same timestamp produce different scores', async () => {
101101
const baseTimestamp = 1700000000000;
102102

103-
const score1 = calculateScore(baseTimestamp, 'msg_001');
104-
const score2 = calculateScore(baseTimestamp, 'msg_002');
105-
const score3 = calculateScore(baseTimestamp, 'msg_003');
103+
const score1 = calculateScore(baseTimestamp, 'msg_001', 'INBOX', 1);
104+
const score2 = calculateScore(baseTimestamp, 'msg_002', 'INBOX', 2);
105+
const score3 = calculateScore(baseTimestamp, 'msg_003', 'INBOX', 3);
106106

107107
assert.notStrictEqual(score1, score2);
108108
assert.notStrictEqual(score2, score3);
@@ -114,41 +114,41 @@ test('Export functionality tests', async t => {
114114
const laterTimestamp = 1700000001000;
115115

116116
// Even with different messageIds, earlier timestamp should have lower score
117-
const scoreEarlier = calculateScore(earlierTimestamp, 'msg_zzz');
118-
const scoreLater = calculateScore(laterTimestamp, 'msg_aaa');
117+
const scoreEarlier = calculateScore(earlierTimestamp, 'msg_zzz', 'INBOX', 1);
118+
const scoreLater = calculateScore(laterTimestamp, 'msg_aaa', 'INBOX', 2);
119119

120120
assert.ok(scoreEarlier < scoreLater, 'Earlier timestamp should produce lower score');
121121
});
122122

123-
await t.test('Score calculation: same messageId produces same tiebreaker', async () => {
123+
await t.test('Score calculation: same inputs produce same score', async () => {
124124
const timestamp = 1700000000000;
125125

126-
const score1 = calculateScore(timestamp, 'consistent_id');
127-
const score2 = calculateScore(timestamp, 'consistent_id');
126+
const score1 = calculateScore(timestamp, 'consistent_id', 'INBOX', 100);
127+
const score2 = calculateScore(timestamp, 'consistent_id', 'INBOX', 100);
128128

129-
assert.strictEqual(score1, score2, 'Same messageId should produce same score');
129+
assert.strictEqual(score1, score2, 'Same inputs should produce same score');
130130
});
131131

132132
await t.test('Score calculation: handles Date objects', async () => {
133133
const date = new Date(1700000000000);
134134
const timestamp = 1700000000000;
135135
const messageId = 'msg_test';
136136

137-
const scoreFromDate = calculateScore(date, messageId);
138-
const scoreFromTimestamp = calculateScore(timestamp, messageId);
137+
const scoreFromDate = calculateScore(date, messageId, 'INBOX', 1);
138+
const scoreFromTimestamp = calculateScore(timestamp, messageId, 'INBOX', 1);
139139

140140
assert.strictEqual(scoreFromDate, scoreFromTimestamp, 'Date object and timestamp should produce same score');
141141
});
142142

143143
await t.test('Score calculation: handles null/undefined/empty messageId', async () => {
144144
const timestamp = 1700000000000;
145145

146-
const scoreNull = calculateScore(timestamp, null);
147-
const scoreUndefined = calculateScore(timestamp, undefined);
148-
const scoreEmpty = calculateScore(timestamp, '');
146+
const scoreNull = calculateScore(timestamp, null, null, null);
147+
const scoreUndefined = calculateScore(timestamp, undefined, undefined, undefined);
148+
const scoreEmpty = calculateScore(timestamp, '', '', '');
149149

150-
assert.strictEqual(scoreNull, scoreEmpty, 'null messageId should be treated as empty');
151-
assert.strictEqual(scoreUndefined, scoreEmpty, 'undefined messageId should be treated as empty');
150+
assert.strictEqual(scoreNull, scoreEmpty, 'null inputs should be treated as empty');
151+
assert.strictEqual(scoreUndefined, scoreEmpty, 'undefined inputs should be treated as empty');
152152
});
153153

154154
await t.test('Score calculation: long messageIds work correctly', async () => {
@@ -158,8 +158,8 @@ test('Export functionality tests', async t => {
158158
const outlookId = 'AAMkAGVmMDEzMTM4LTZmYWUtNDdkNC1hMDZiLTU1OGY5OTZhYmY4OABGAAAAAADUuTJK1K9sTpCdqXop_4NaBwCd9nJ-tVysQYj2Cekan9XRAAAAAAEMAAC';
159159
const gmailId = '18abc123def456789';
160160

161-
const outlookScore = calculateScore(timestamp, outlookId);
162-
const gmailScore = calculateScore(timestamp, gmailId);
161+
const outlookScore = calculateScore(timestamp, outlookId, 'INBOX', 1);
162+
const gmailScore = calculateScore(timestamp, gmailId, 'INBOX', 2);
163163

164164
assert.strictEqual(typeof outlookScore, 'number');
165165
assert.strictEqual(typeof gmailScore, 'number');
@@ -171,10 +171,10 @@ test('Export functionality tests', async t => {
171171
const timestamp = 1700000000000;
172172
const messageIds = ['msg_001', 'msg_002', 'msg_003', 'msg_004', 'msg_005', 'msg_100', 'msg_200', 'msg_300', 'msg_400', 'msg_500'];
173173

174-
const scores = messageIds.map(id => calculateScore(timestamp, id));
174+
const scores = messageIds.map((id, i) => calculateScore(timestamp, id, 'INBOX', i + 1));
175175
const uniqueScores = new Set(scores);
176176

177-
assert.strictEqual(uniqueScores.size, messageIds.length, 'All messageIds should produce unique scores');
177+
assert.strictEqual(uniqueScores.size, messageIds.length, 'All messages should produce unique scores');
178178
});
179179

180180
// formatStatus tests

workers/export.js

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ async function exportMessages(job, exportData) {
311311
});
312312

313313
const gzipStream = zlib.createGzip();
314-
const fileStream = fs.createWriteStream(filePath);
314+
const fileStream = fs.createWriteStream(filePath, job.data.isResumed ? { flags: 'a' } : undefined);
315315

316316
let streamError = null;
317317
function handleStreamError(err) {
@@ -605,10 +605,21 @@ async function exportMessages(job, exportData) {
605605
processingError = err;
606606
}
607607

608+
const FINALIZATION_TIMEOUT = 30000;
608609
await new Promise((resolve, reject) => {
610+
const timeout = setTimeout(() => {
611+
fileStream.destroy();
612+
reject(streamError || new Error('Stream finalization timed out'));
613+
}, FINALIZATION_TIMEOUT);
609614
gzipStream.end();
610-
fileStream.once('finish', resolve);
611-
fileStream.once('error', err => reject(streamError || err));
615+
fileStream.once('finish', () => {
616+
clearTimeout(timeout);
617+
resolve();
618+
});
619+
fileStream.once('error', err => {
620+
clearTimeout(timeout);
621+
reject(streamError || err);
622+
});
612623
});
613624

614625
if (processingError) {

0 commit comments

Comments
 (0)