Skip to content

Commit b5070eb

Browse files
bert-ebert-e
committed
Merge branch 'bugfix/S3UTILS-192-objectRepairCrash' into tmp/octopus/w/1.16/bugfix/S3UTILS-192-objectRepairCrash
2 parents ba16cc2 + 075dcff commit b5070eb

File tree

3 files changed

+114
-22
lines changed

3 files changed

+114
-22
lines changed

‎ObjectRepair/DuplicateKeysIngestion.js‎

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
const { waterfall } = require('async');
22
const { Logger } = require('werelogs');
3+
const { errors } = require('arsenal');
34
const { httpRequest } = require('../repairDuplicateVersionsSuite');
45
const { SproxydKeysProcessor } = require('./DuplicateKeysWindow');
56
const { ProxyLoggerCreator, AggregateLogger } = require('./Logging');
@@ -73,10 +74,26 @@ class RaftJournalReader {
7374
log.error('unable to fetch cseq', { err, requestUrl });
7475
return cb(err);
7576
}
77+
if (res.statusCode === 416) {
78+
// RAFT session is empty: start at 1
79+
this.cseq = 1;
80+
} else {
81+
if (res.statusCode !== 200) {
82+
log.error(
83+
'unable to fetch cseq: error HTTP status code returned',
84+
{ statusCode: res.statusCode },
85+
);
86+
return cb(errors.InternalError);
87+
}
7688

77-
const body = JSON.parse(res.body);
78-
79-
this._setCseq(body);
89+
try {
90+
const body = JSON.parse(res.body);
91+
this._setCseq(body);
92+
} catch (err) {
93+
log.error('unable to fetch cseq: invalid JSON in response body');
94+
return cb(errors.InternalError);
95+
}
96+
}
8097

8198
// make sure begin is at least 1 since Raft Journal logs are 1-indexed
8299
this.begin = Math.max(1, this.cseq - this.lookBack);
@@ -134,7 +151,12 @@ class RaftJournalReader {
134151
return cb(new Error(`GET ${requestUrl} returned empty body at ${this.begin}`));
135152
}
136153

137-
const body = JSON.parse(res.body);
154+
let body;
155+
try {
156+
body = JSON.parse(res.body);
157+
} catch (err) {
158+
return cb(new Error(`GET ${requestUrl}: invalid JSON in response body`));
159+
}
138160
// FIXME this special case should be taken care of once S3C-3928 is fixed
139161
if (body.log.length === 0) {
140162
return cb(new RangeError(`GET ${requestUrl} found no new records at ${this.begin}`));

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "s3utils",
3-
"version": "1.15.8",
3+
"version": "1.15.9",
44
"engines": {
55
"node": ">= 22"
66
},

‎tests/unit/SproxydKeysScan/DuplicateKeysIngestionTest.js‎

Lines changed: 87 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
11
const fs = require('fs');
2+
const { errors } = require('arsenal');
23
const { RaftJournalReader } = require('../../../ObjectRepair/DuplicateKeysIngestion');
34
const { subscribers } = require('../../../ObjectRepair/SproxydKeysSubscribers');
45

56
function getMockResponse(mockStatusCode) {
6-
const mockBody = fs.readFileSync(`${__dirname}/RaftJournalTestData.json`, 'utf8');
77
const mockResponse = {
8-
body: mockBody,
98
statusCode: mockStatusCode,
109
};
10+
if (Math.floor(mockStatusCode / 100) === 2) {
11+
const mockBody = fs.readFileSync(`${__dirname}/RaftJournalTestData.json`, 'utf8');
12+
mockResponse.body = mockBody;
13+
} else {
14+
mockResponse.body = '';
15+
}
1116
return mockResponse;
1217
}
1318

@@ -39,42 +44,88 @@ describe('RaftJournalReader', () => {
3944
describe('::setBegin', () => {
4045
const reader = setupJournalReader();
4146

42-
beforeEach(() => {
43-
reader._httpRequest = mockHttpRequest(null, getMockResponse(200));
44-
});
45-
4647
afterEach(() => {
4748
reader._httpRequest.mockReset();
4849
});
4950

50-
test('when begin is undefined, begin = latest cseq - lookBack', () => {
51+
test('when begin is undefined, begin = latest cseq - lookBack', done => {
52+
reader._httpRequest = mockHttpRequest(null, getMockResponse(200));
5153
reader.begin = undefined;
5254
reader.lookBack = 5;
5355

5456
reader.setBegin(err => {
5557
expect(err).toBe(undefined);
5658
expect(reader.begin).toEqual(reader.cseq - reader.lookBack);
59+
done();
5760
});
5861
});
5962

60-
test('begin is at minimum 1, even with lookback > latest cseq', () => {
63+
test('begin is at minimum 1, even with lookback > latest cseq', done => {
64+
reader._httpRequest = mockHttpRequest(null, getMockResponse(200));
6165
reader.begin = undefined;
6266
reader.lookBack = Infinity;
6367

6468
reader.setBegin(err => {
6569
expect(err).toBe(undefined);
6670
expect(reader.begin).toEqual(1);
71+
done();
6772
});
6873
});
6974

70-
test('if begin is set from a previous call, use the existing begin and ignore lookBack', () => {
75+
test('if begin is set from a previous call, use the existing begin and ignore lookBack', done => {
76+
reader._httpRequest = mockHttpRequest(null, getMockResponse(200));
7177
reader.begin = 3;
7278
reader.lookBack = Infinity;
7379

7480
reader.setBegin(err => {
7581
expect(err).toBe(undefined);
7682
expect(reader._httpRequest).not.toHaveBeenCalled();
7783
expect(reader.begin).toEqual(3);
84+
done();
85+
});
86+
});
87+
88+
test('when begin is undefined and route returns an unexpected error', done => {
89+
reader._httpRequest = mockHttpRequest(new Error('OOPS'), undefined);
90+
reader.begin = undefined;
91+
92+
reader.setBegin(err => {
93+
expect(err).toEqual(new Error('OOPS'));
94+
done();
95+
});
96+
});
97+
98+
test('when begin is undefined and route returns a 416 HTTP status with empty response body', done => {
99+
reader._httpRequest = mockHttpRequest(null, getMockResponse(416));
100+
reader.begin = undefined;
101+
102+
reader.setBegin(err => {
103+
expect(err).toBe(undefined);
104+
expect(reader.begin).toEqual(1);
105+
done();
106+
});
107+
});
108+
109+
test('when begin is undefined and route returns a 500 HTTP status with empty response body', done => {
110+
reader._httpRequest = mockHttpRequest(null, getMockResponse(500));
111+
reader.begin = undefined;
112+
113+
reader.setBegin(err => {
114+
expect(err).toEqual(errors.InternalError);
115+
done();
116+
});
117+
});
118+
119+
test('when begin is undefined and route returns a 200 OK HTTP status with invalid JSON in response body', done => {
120+
reader._httpRequest = mockHttpRequest(null, {
121+
statusCode: 200,
122+
body: '{BADJSON}',
123+
});
124+
reader.begin = undefined;
125+
126+
reader.setBegin(err => {
127+
expect(err).toEqual(errors.InternalError);
128+
done();
78129
});
79130
});
80131
});
@@ -96,7 +147,7 @@ describe('RaftJournalReader', () => {
96147
expect(body).toBe(undefined);
97148
};
98149

99-
test('should correctly read mocked data', () => {
150+
test('should correctly read mocked data', done => {
100151
reader._httpRequest = mockHttpRequest(null, getMockResponse(200));
101152
reader.getBatch((err, body) => {
102153
expect(reader._httpRequest).toHaveBeenCalled();
@@ -109,35 +160,51 @@ describe('RaftJournalReader', () => {
109160
expect(err).toBe(null);
110161
expect(body).not.toBe(null);
111162
expect(body.log).not.toBe(null);
163+
done();
112164
});
113165
});
114166

115-
test('should return error with a non-200 status code', () => {
167+
test('should return error with a non-200 status code', done => {
116168
reader._httpRequest = mockHttpRequest(null, getMockResponse(500));
117169
reader.getBatch((err, body) => {
118170
returnedError(err, body);
171+
done();
119172
});
120173
});
121174

122-
test('should return error with a missing body', () => {
175+
test('should return error with a missing body', done => {
123176
reader._httpRequest = mockHttpRequest(null, { statusCode: 200 });
124177
reader.getBatch((err, body) => {
125178
returnedError(err, body);
179+
done();
126180
});
127181
});
128182

129-
test('should return error with null response', () => {
183+
test('should return error with null response', done => {
130184
reader._httpRequest = mockHttpRequest(null, null);
131185
reader.getBatch((err, body) => {
132186
returnedError(err, body);
187+
done();
133188
});
134189
});
135190

136191
// FIXME this special case should be taken care of once S3C-3928 is fixed
137-
test('should return error with response containing empty log', () => {
192+
test('should return error with response containing empty log', done => {
138193
reader._httpRequest = mockHttpRequest(null, getEmptyRsMockResponse());
139194
reader.getBatch((err, body) => {
140195
returnedError(err, body);
196+
done();
197+
});
198+
});
199+
200+
test('should return error with response containing invalid JSON', done => {
201+
reader._httpRequest = mockHttpRequest(null, {
202+
statusCode: 200,
203+
body: '{BADJSON}',
204+
});
205+
reader.getBatch((err, body) => {
206+
returnedError(err, body);
207+
done();
141208
});
142209
});
143210
});
@@ -211,14 +278,15 @@ describe('RaftJournalReader', () => {
211278
reader._httpRequest = mockHttpRequest(null, getMockResponse(200));
212279
afterEach(() => reader._httpRequest.mockReset());
213280

214-
test('inserts correct sproxyd key data into SproxydKeyProcessor', () => {
281+
test('inserts correct sproxyd key data into SproxydKeyProcessor', done => {
215282
expect(reader.begin).toEqual(1);
216283
reader.runOnce((err, timeout) => {
217284
expect(err).toBe(null);
218285
expect(timeout).toBe(0);
219286
expect(reader._httpRequest).toHaveBeenCalled();
220287
expect(reader.processor.insert).toHaveBeenCalled();
221288
expect(reader.begin).toBeGreaterThan(1);
289+
done();
222290
});
223291
});
224292

@@ -229,19 +297,21 @@ describe('RaftJournalReader', () => {
229297
expect(reader.processor.insert).not.toHaveBeenCalled();
230298
};
231299

232-
test('should set a timeout of 5000 milliseconds when there is any error during ingestion', () => {
300+
test('should set a timeout of 5000 milliseconds when there is any error during ingestion', done => {
233301
reader._httpRequest = mockHttpRequest(null, getMockResponse(500));
234302
reader.processor.insert.mockReset();
235303
reader.runOnce((err, timeout) => {
236304
waitsFiveSeconds(err, timeout);
305+
done();
237306
});
238307
});
239308

240-
test('should set a timeout of 5000 milliseconds when there is no new data from raft journal', () => {
309+
test('should set a timeout of 5000 milliseconds when there is no new data from raft journal', done => {
241310
reader._httpRequest = mockHttpRequest(null, null);
242311
reader.processor.insert.mockReset();
243312
reader.runOnce((err, timeout) => {
244313
waitsFiveSeconds(err, timeout);
314+
done();
245315
});
246316
});
247317
});

0 commit comments

Comments
 (0)