Skip to content

Commit ea7f386

Browse files
committed
fix(ts-sdk): add error handling for JSON.parse and use async file I/O in ModelClient
- Add try/catch for JSON.parse in parseRequestLine and parseResponseLine to handle corrupted log files gracefully with meaningful error messages - Replace readFileSync/appendFileSync with async readFile/appendFile from fs/promises to avoid blocking the event loop - Re-throw parse errors immediately in popRequest instead of retrying - Add tests for JSON parse error handling and async file I/O usage Fixes PR alibaba#492 review feedback
1 parent 706ceba commit ea7f386

File tree

2 files changed

+92
-22
lines changed

2 files changed

+92
-22
lines changed

rock/ts-sdk/src/model/client.test.ts

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,59 @@ describe('ModelClient timeout and cancellation', () => {
159159
});
160160
});
161161

162+
describe('async file I/O', () => {
163+
it('should import readFile from fs/promises (not readFileSync from fs)', async () => {
164+
// Read the client.ts source to verify imports
165+
const { readFileSync: readSource } = await import('fs');
166+
const sourceCode = readSource(require.resolve('./client.js'), 'utf-8');
167+
168+
// Should import from fs/promises
169+
expect(sourceCode).toMatch(/from ['"]fs\/promises['"]/);
170+
171+
// Should import readFile
172+
expect(sourceCode).toMatch(/import\s+\{[^}]*readFile[^}]*\}\s+from\s+['"]fs\/promises['"]/);
173+
174+
// Should NOT import readFileSync
175+
expect(sourceCode).not.toMatch(/readFileSync/);
176+
});
177+
178+
it('should import appendFile from fs/promises (not appendFileSync from fs)', async () => {
179+
// Read the client.ts source to verify imports
180+
const { readFileSync: readSource } = await import('fs');
181+
const sourceCode = readSource(require.resolve('./client.js'), 'utf-8');
182+
183+
// Should import appendFile from fs/promises
184+
expect(sourceCode).toMatch(/import\s+\{[^}]*appendFile[^}]*\}\s+from\s+['"]fs\/promises['"]/);
185+
186+
// Should NOT import appendFileSync
187+
expect(sourceCode).not.toMatch(/appendFileSync/);
188+
});
189+
});
190+
191+
describe('JSON parse error handling', () => {
192+
it('should throw meaningful error when request line has corrupted JSON', async () => {
193+
// Create log file with corrupted JSON in meta section
194+
const content = `${REQUEST_START_MARKER}test-request${REQUEST_END_MARKER}{invalid json}\n`;
195+
writeFileSync(logFile, content);
196+
197+
// Should throw "Invalid request line format" immediately
198+
// NOT retry until timeout, NOT throw raw SyntaxError
199+
await expect(client.popRequest(1, { timeout: 5 })).rejects.toThrow('Invalid request line format');
200+
});
201+
202+
it('should throw meaningful error when response line has corrupted JSON', async () => {
203+
// Create log file with request and corrupted response
204+
const RESPONSE_START_MARKER = '__RESPONSE_START__';
205+
const RESPONSE_END_MARKER = '__RESPONSE_END__';
206+
const content = `${REQUEST_START_MARKER}test-request${REQUEST_END_MARKER}{"index":0}\n${RESPONSE_START_MARKER}response${RESPONSE_END_MARKER}{bad}\n`;
207+
writeFileSync(logFile, content);
208+
209+
// pushResponse internally calls parseResponseLine
210+
// Should throw "Invalid response line format"
211+
await expect(client.pushResponse(1, 'new-response')).rejects.toThrow('Invalid response line format');
212+
});
213+
});
214+
162215
describe('backward compatibility', () => {
163216
it('should use default timeout when not specified', async () => {
164217
// This test verifies backward compatibility

rock/ts-sdk/src/model/client.ts

Lines changed: 39 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,8 @@
22
* Model client for LLM interaction
33
*/
44

5-
import { existsSync, readFileSync, appendFileSync } from 'fs';
5+
import { existsSync } from 'fs';
6+
import { readFile, appendFile } from 'fs/promises';
67
import { initLogger } from '../logger.js';
78
import { envVars } from '../env_vars.js';
89
import { sleep } from '../utils/retry.js';
@@ -82,7 +83,7 @@ export class ModelClient {
8283
const lastResponseLine = await this.readLastResponseLine();
8384

8485
if (lastResponseLine === null) {
85-
this.appendResponse(content);
86+
await this.appendResponse(content);
8687
return;
8788
}
8889

@@ -98,7 +99,7 @@ export class ModelClient {
9899
return;
99100
}
100101

101-
this.appendResponse(content);
102+
await this.appendResponse(content);
102103
}
103104

104105
/**
@@ -139,9 +140,15 @@ export class ModelClient {
139140
logger.debug(`Last request is not the index ${index} we want, waiting...`);
140141
await sleep(1000);
141142
} catch (e) {
142-
// Re-throw abort errors
143-
if (e instanceof Error && e.message.includes('aborted')) {
144-
throw e;
143+
// Re-throw abort errors and parse errors immediately
144+
if (e instanceof Error) {
145+
if (e.message.includes('aborted')) {
146+
throw e;
147+
}
148+
// Re-throw parse errors (invalid format) immediately - don't retry
149+
if (e.message.includes('Invalid request line format')) {
150+
throw e;
151+
}
145152
}
146153
// For other errors (like file not found), wait and retry
147154
logger.debug(`Error reading request: ${e}, waiting...`);
@@ -177,7 +184,7 @@ export class ModelClient {
177184
continue;
178185
}
179186

180-
const content = readFileSync(this.logFile, 'utf-8');
187+
const content = await readFile(this.logFile, 'utf-8');
181188
const lines = content.split('\n').filter((l) => l.trim());
182189

183190
if (lines.length === 0) {
@@ -195,25 +202,35 @@ export class ModelClient {
195202
return { requestJson: SESSION_END_MARKER, meta: {} };
196203
}
197204

198-
const parts = lineContent.split(REQUEST_END_MARKER);
199-
const metaJson = parts[1] ?? '';
200-
const requestJson = parts[0]?.split(REQUEST_START_MARKER)[1] ?? '';
201-
const meta = JSON.parse(metaJson);
205+
try {
206+
const parts = lineContent.split(REQUEST_END_MARKER);
207+
const metaJson = parts[1] ?? '';
208+
const requestJson = parts[0]?.split(REQUEST_START_MARKER)[1] ?? '';
209+
const meta = JSON.parse(metaJson);
202210

203-
return { requestJson, meta };
211+
return { requestJson, meta };
212+
} catch (e) {
213+
logger.error(`Failed to parse request line: ${lineContent}, error: ${e}`);
214+
throw new Error(`Invalid request line format: ${e}`);
215+
}
204216
}
205217

206218
private parseResponseLine(lineContent: string): { responseJson: string; meta: Record<string, unknown> } {
207-
const parts = lineContent.split(RESPONSE_END_MARKER);
208-
const metaJson = parts[1] ?? '';
209-
const responseJson = parts[0]?.split(RESPONSE_START_MARKER)[1] ?? '';
210-
const meta = JSON.parse(metaJson);
211-
212-
return { responseJson, meta };
219+
try {
220+
const parts = lineContent.split(RESPONSE_END_MARKER);
221+
const metaJson = parts[1] ?? '';
222+
const responseJson = parts[0]?.split(RESPONSE_START_MARKER)[1] ?? '';
223+
const meta = JSON.parse(metaJson);
224+
225+
return { responseJson, meta };
226+
} catch (e) {
227+
logger.error(`Failed to parse response line: ${lineContent}, error: ${e}`);
228+
throw new Error(`Invalid response line format: ${e}`);
229+
}
213230
}
214231

215232
private async readLastRequestLine(): Promise<string> {
216-
const content = readFileSync(this.logFile, 'utf-8');
233+
const content = await readFile(this.logFile, 'utf-8');
217234
const lines = content.split('\n').filter((l) => l.trim());
218235

219236
for (let i = lines.length - 1; i >= 0; i--) {
@@ -227,7 +244,7 @@ export class ModelClient {
227244
}
228245

229246
private async readLastResponseLine(): Promise<string | null> {
230-
const content = readFileSync(this.logFile, 'utf-8');
247+
const content = await readFile(this.logFile, 'utf-8');
231248
const lines = content.split('\n').filter((l) => l.trim());
232249

233250
for (let i = lines.length - 1; i >= 0; i--) {
@@ -240,8 +257,8 @@ export class ModelClient {
240257
return null;
241258
}
242259

243-
private appendResponse(content: string): void {
244-
appendFileSync(this.logFile, content);
260+
private async appendResponse(content: string): Promise<void> {
261+
await appendFile(this.logFile, content);
245262
}
246263

247264
private constructResponse(lastResponse: string, index: number): string {

0 commit comments

Comments
 (0)