Fix error when a not yet inserted job is updated #7196

davimacedo · 2021-02-16T08:35:20Z

New Pull Request Checklist

[ X ] I am not disclosing a vulnerability.
[ X ] I am creating this PR in reference to an issue.

Issue Description

An error can happen if you try to update a job that was just created. Because of this we have flaky tests. The error is easier to reproduce if you change the job message but it can happen even when it completes fast and the status needs to be changed.

Related issue: #7180

Approach

I actually investigated a lot our code and did a lot of tests regarding write concerns, etc, but I didn't find the root cause. I actually believe the problem is not in our side but it is probably a bug on MongoDB 4.4. The solution I used was just upsert the job status when updating it. Since it fixes the problem and I can't see any downside, I believe it is an acceptable solution.

TODOs before merging

Add test cases
[ X ] Add entry to changelog
Add changes to documentation (guides, repository pages, in-code descriptions)
Add security check
Add new Parse Error codes to Parse JS SDK
...

codecov · 2021-02-16T08:42:21Z

Codecov Report

Merging #7196 (07187c0) into master (a430d6f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #7196   +/-   ##
=======================================
  Coverage   94.02%   94.02%           
=======================================
  Files         172      172           
  Lines       12867    12867           
=======================================
  Hits        12098    12098           
  Misses        769      769

Impacted Files	Coverage Δ
src/Controllers/DatabaseController.js	`95.46% <0.00%> (-0.15%)`	⬇️
src/Adapters/Files/GridFSBucketAdapter.js	`80.32% <0.00%> (+0.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a430d6f...82a84f9. Read the comment docs.

mtrezza

I actually believe the problem is not in our side but it is probably a bug on MongoDB 4.4. The solution I used was just upsert the job status when updating it.

LGTM, I think we could find out whether it is a MongoDB 4.4 bug if we just run a loop of saving and immediately updating an obj, bypassing first the Parse.Job logic, then bypassing the DB adapter.

Could there be a scenario in which the upsert creates a job that has already been deleted due to a race conditions? I think that should fail gracefully.

mtrezza · 2021-02-16T09:42:53Z

Actually, looking at the current run and the error in Postgres, it seems to be the same issue with a different object, so it’s probably not MongoDB related.

Cloud Code cloud jobs should define a job
  Message:
    Unhandled promise rejection: error: relation "_JobStatus" does not exist (line 1180)
  Stack:
    error properties: Object({ length: 108, severity: 'ERROR', code: '42P01', detail: undefined, hint: undefined, position: '8', internalPosition: undefined, internalQuery: undefined, where: undefined, schema: undefined, table: undefined, dataType: undefined, constraint: undefined, file: 'parse_relation.c', routine: 'parserOpenTable' })
        at <Jasmine>
        at Parser.parseErrorMessage (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
        at Parser.handlePacket (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
        at Parser.parse (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
        at Socket.stream.on (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
        at Socket.emit (events.js:198:13)
        at Socket.EventEmitter.emit (domain.js:448:20)
        at addChunk (_stream_readable.js:288:12)
        at readableAddChunk (_stream_readable.js:269:11)
        at Socket.Readable.push (_stream_readable.js:224:10)
        at TCP.onStreamRead [as onread] (internal/stream_base_commons.js:94:17)

davimacedo · 2021-02-16T21:56:49Z

I actually believe the problem is not in our side but it is probably a bug on MongoDB 4.4. The solution I used was just upsert the job status when updating it.

LGTM, I think we could find out whether it is a MongoDB 4.4 bug if we just run a loop of saving and immediately updating an obj, bypassing first the Parse.Job logic, then bypassing the DB adapter.

Could there be a scenario in which the upsert creates a job that has already been deleted due to a race conditions? I think that should fail gracefully.

I don't think so. I am not aware of any code in place that deletes JobStatus. The developers could maybe write a custom code and delete a JobStatus using the master key. I believe that's not a common scenario though.

I will try what you suggested anyways and I will check different versions of mongo

davimacedo · 2021-02-16T21:57:41Z

Actually, looking at the current run and the error in Postgres, it seems to be the same issue with a different object, so it’s probably not MongoDB related.

Cloud Code cloud jobs should define a job
  Message:
    Unhandled promise rejection: error: relation "_JobStatus" does not exist (line 1180)
  Stack:
    error properties: Object({ length: 108, severity: 'ERROR', code: '42P01', detail: undefined, hint: undefined, position: '8', internalPosition: undefined, internalQuery: undefined, where: undefined, schema: undefined, table: undefined, dataType: undefined, constraint: undefined, file: 'parse_relation.c', routine: 'parserOpenTable' })
        at <Jasmine>
        at Parser.parseErrorMessage (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
        at Parser.handlePacket (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
        at Parser.parse (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
        at Socket.stream.on (/home/runner/work/parse-server/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
        at Socket.emit (events.js:198:13)
        at Socket.EventEmitter.emit (domain.js:448:20)
        at addChunk (_stream_readable.js:288:12)
        at readableAddChunk (_stream_readable.js:269:11)
        at Socket.Readable.push (_stream_readable.js:224:10)
        at TCP.onStreamRead [as onread] (internal/stream_base_commons.js:94:17)

I beleive this one is another Flaky test. I will take a look at it as well.

davimacedo · 2021-02-18T08:47:07Z

After a really hard time understanding what was happening it turned out that the problem was in our side. After every test-case, we delete all the data, but, the process of saving the message and status of a job is asynchronous. So it was happening that, in some cases, the test was finishing (and the data getting deleted) before the status and message update.

mtrezza · 2021-02-18T09:46:52Z

Was the test case not async/await?

it(... async () {
});

I think that would not end the test and delete the data before the test is done. Or even if it is not async/await, the test wouldn't finish before done() is called.

davimacedo · 2021-02-18T18:14:14Z

The tests currently are like this:

it('should define a job', done => {
      expect(() => {
        Parse.Cloud.job('myJob', () => {});
      }).not.toThrow();

      request({
        method: 'POST',
        url: 'http://localhost:8378/1/jobs/myJob',
        headers: {
          'X-Parse-Application-Id': Parse.applicationId,
          'X-Parse-Master-Key': Parse.masterKey,
        },
      }).then(
        () => {
          done();
        },
        err => {
          fail(err);
          done();
        }
      );
    });

Basically they do the api call to start the job and then call the done() function to finish the test. The problem happens because this api only starts the job and leaves it running in background as you can see here. So sometimes the done() function is called and the data is deleted before the job running in background has actually completed.

The changes in this PR makes the tests to wait for the job completition.

mtrezza · 2021-02-18T18:23:29Z

Makes sense now, thanks. Hats off if that finally fixes these tests. 🤞

* Fix error when a not yet inserted job is updated * Add entry to changelog * revert the upsert change and fix the test * Revert the change so job execute a single time * Fix other tests with potential similar problem

parseplatformorg · 2021-11-01T16:33:33Z

🎉 This change has been released in version 5.0.0-beta.1

parseplatformorg · 2022-03-14T13:05:50Z

🎉 This change has been released in version 5.0.0

davimacedo added 2 commits February 16, 2021 00:29

Fix error when a not yet inserted job is updated

2c1d672

Add entry to changelog

595e4c7

davimacedo mentioned this pull request Feb 16, 2021

Fix flaky tests #7180

Closed

4 tasks

davimacedo requested review from mtrezza and dplewis February 16, 2021 09:06

mtrezza approved these changes Feb 16, 2021

View reviewed changes

davimacedo added 3 commits February 18, 2021 00:16

revert the upsert change and fix the test

cf83992

Revert the change so job execute a single time

703c396

Fix other tests with potential similar problem

bc771fa

Merge branch 'upstream' into fixFlakyTest2

ef880da

Merge branch 'upstream' into fixFlakyTest2

82a84f9

davimacedo merged commit 603cc1f into parse-community:master Feb 18, 2021

dplewis mentioned this pull request Feb 20, 2021

Ensure pushStatus is properly running #7213

Merged

8 tasks

parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021

mtrezza mentioned this pull request Mar 12, 2022

build: release 5.0 #7851

Merged

parseplatformorg added the state:released Released as stable version label Mar 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error when a not yet inserted job is updated #7196

Fix error when a not yet inserted job is updated #7196

davimacedo commented Feb 16, 2021

codecov bot commented Feb 16, 2021 •

edited

Loading

mtrezza left a comment •

edited

Loading

mtrezza commented Feb 16, 2021 •

edited

Loading

davimacedo commented Feb 16, 2021 •

edited

Loading

davimacedo commented Feb 16, 2021

davimacedo commented Feb 18, 2021

mtrezza commented Feb 18, 2021 •

edited

Loading

davimacedo commented Feb 18, 2021 •

edited

Loading

mtrezza commented Feb 18, 2021

parseplatformorg commented Nov 1, 2021

parseplatformorg commented Mar 14, 2022

Fix error when a not yet inserted job is updated #7196

Fix error when a not yet inserted job is updated #7196

Conversation

davimacedo commented Feb 16, 2021

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

codecov bot commented Feb 16, 2021 • edited Loading

Codecov Report

mtrezza left a comment • edited Loading

Choose a reason for hiding this comment

mtrezza commented Feb 16, 2021 • edited Loading

davimacedo commented Feb 16, 2021 • edited Loading

davimacedo commented Feb 16, 2021

davimacedo commented Feb 18, 2021

mtrezza commented Feb 18, 2021 • edited Loading

davimacedo commented Feb 18, 2021 • edited Loading

mtrezza commented Feb 18, 2021

parseplatformorg commented Nov 1, 2021

parseplatformorg commented Mar 14, 2022

codecov bot commented Feb 16, 2021 •

edited

Loading

mtrezza left a comment •

edited

Loading

mtrezza commented Feb 16, 2021 •

edited

Loading

davimacedo commented Feb 16, 2021 •

edited

Loading

mtrezza commented Feb 18, 2021 •

edited

Loading

davimacedo commented Feb 18, 2021 •

edited

Loading