fix: throw an error instead of ignoring it when uploading batches#207
fix: throw an error instead of ignoring it when uploading batches#207gasperzgonec wants to merge 9 commits into
Conversation
radovanjorgic
changed the title
gasperzgonec
force-pushed
the
gasperz/ASCPT-28
branch
from
27ba842 to
7a4fc6e
Compare
|
|
| const isSdkLog = message.payload?.isSdkLog ?? true; | ||
| this.logger.logFn(stringifiedArgs, level, isSdkLog); | ||
| if (typeof this.logger?.logFn === 'function') { | ||
| this.logger.logFn(stringifiedArgs, level, isSdkLog); |
There was a problem hiding this comment.
Hmm, why was this needed? Isn't logFn always a function?
There was a problem hiding this comment.
I got some errors and added this to get rid of those. It's probably related to the fixture tests. Removing this.
| console.log( | ||
| `Uploading all repos before emitting event with event type: ${newEventType}.` | ||
| ); | ||
| const canEmit = await this.beforeEmit(newEventType); |
There was a problem hiding this comment.
canEmit calls the beforeEmit function (confusing)? And if it returns false what happens?
There was a problem hiding this comment.
Renamed to a local prepared boolean and documented the return contract on beforeEmit's docstring. beforeEmit no longer emits anything itself; see the answer below, it's simpler now.
| parentPort?.postMessage(WorkerMessageSubject.WorkerMessageExit); | ||
| this.hasWorkerEmitted = true; | ||
| const payload = this.buildEmitPayload(newEventType, data); | ||
| const sent = await this.sendToPlatform(newEventType, payload); |
There was a problem hiding this comment.
Naming is a bit strange here IMO, can we find better name for sendToPlatform? And again, if it fails we just return, is that correct?
Maybe we could move the actual emit function here, so we have beforeEmit, actual emit logic and afterEmit in this exposed method.
There was a problem hiding this comment.
Removed sendToPlatform/emitToPlatform entirely. The actual emit() call (from control-protocol.ts) is now inlined directly in the public emit() method, right after beforeEmit/buildEmitPayload, with its own try/catch for the WorkerMessageEmitted/WorkerMessageExit outcome — matching what you did in v2 (beforeEmit → build payload → actual emit → afterEmit, all in one method).
| } catch (error) { | ||
| console.error('Error while uploading repos', error); | ||
| for (const repo of this.repos) { | ||
| this.artifacts = [...this.artifacts, ...repo.uploadedArtifacts]; |
There was a problem hiding this comment.
What is the point of this on error? We extend the artifacts array with repo.uploadedArtifacts? Can you explain?
There was a problem hiding this comment.
When uploadAllRepos() throws partway through (e.g. repo 3 of 5 fails), repos 1-2 already had their artifacts merged into this.artifacts by the loop inside uploadAllRepos, but the loop aborts on the failing repo. This second loop re-scans every repo's uploadedArtifacts and merges them via the artifacts setter (which dedupes by object identity, so already-added ones aren't duplicated), guaranteeing every successfully-uploaded artifact is reported in the error emit instead of becoming an orphan in storage.
| const { eventType: errorEventType } = getTimeoutErrorEventType( | ||
| this.event.payload.event_type | ||
| ); | ||
| await this.emitUploadFailure(errorEventType, error); |
There was a problem hiding this comment.
Just curious if we need separate function for this.
There was a problem hiding this comment.
Removed, now it's inlined, as it was only used in one place.
|
- Revert defensive logFn guard in spawn.ts (unrelated to this fix, logFn is always a function on the real Logger). - Rename canEmit to prepared and document beforeEmit's return contract. - Collapse sendToPlatform/afterEmit into a single emitToPlatform method, matching the beforeEmit/buildEmitPayload/afterEmit shape used in v2. - Inline emitUploadFailure into beforeEmit's catch block since it had a single call site. - Run eslint --fix to clean up pre-existing formatting violations.
|
Matches v2's adapter shape (beforeEmit/buildEmitPayload/afterEmit, no separate sendToPlatform/emitToPlatform helper) per review feedback. beforeEmit no longer swallows the repo-upload failure itself; emit() catches it, collects partial artifacts, and picks the error event type before sending, so there's a single call site for the actual platform emit and its WorkerMessageEmitted/WorkerMessageExit outcome handling.
|
2 participants
Description
Connected Issues
Checklist
npm run testOR no tests needed.npm run test:backwards-compatibility.npm run lint.