Conversation
there is an issue with how the course ids are being generated that is causing duplicates to be generated consistently thus preventing the creation of new courses for the site. I detailed more of this in `api/src/utils/id_generator.ts`. for now this will keep track of created course ids and pre-existing ones so that we can relyably create new ones without having to hit the database all the time and prevent potential race conditions by having each request calculate it.
fixes a type error, not sure where the type is so that it can be specified so it is being inferred.
| for (let key of Object.keys(tmp_ids)) { | ||
| if (now - tmp_ids[key].ts > max_lifetime) { | ||
| ids_index.delete(tmp_ids[key].id); | ||
| tmp_ids.delete(key); | ||
| deleted += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
tmp_ids is a Map, but this code is iterating it like a plain object. Object.keys(tmp_ids) returns [], so cleanup never runs and stale temporary reservations never expire, so leaked IDs stay blocked for the lifetime of the process.
Can we iterate the map entries directly instead, e.g. for (const [key, tmp] of tmp_ids)?
Running:
node -e "const tmp_ids=new Map([['course-a',{id:1,ts:Date.now()-60000}]]); console.log('Object.keys(tmp_ids)=', Object.keys(tmp_ids)); console.log('Map entries=', [...tmp_ids.entries()]);"
Produces:
Object.keys(tmp_ids)= []
Map entries= [ [ 'course-a', { id: 1, ts: 1775528671920 } ] ]
which shows that while the Map entries does indeed contain course-a, but Object.keys(...) cannot see the items stored inside the Map.
There was a problem hiding this comment.
this is what I get for making quick changes. it should be iterating through the tmp_ids properly
| let id = 0; | ||
|
|
||
| try { | ||
| let result = sequelize.transaction(async (transaction) => { |
There was a problem hiding this comment.
This starts the transaction but does not wait for it to finish. As a result, commit_id(id) below can run while id is still 0, and before Sequelize has committed the transaction. If the transaction later rejects, the surrounding catch also won’t reliably perform the intended rollback. Can we await the transaction result before finalizing the generated id?
There was a problem hiding this comment.
added in the missing await for the return of the transaction.
- when creating a new course, the promise was not being `await`ed and the id was thusly not being commited - changed how the tmp id cleanup was handled to use the built-in iterator to fix an issue with not properly iterating over the temp ids - updated the log for the tmp id cleanup to output how many ids it cleaned up - removed the return from `add_name` as it is not used and would return the `Set` which is not desired.
NickK21
left a comment
NickK21
left a comment
There was a problem hiding this comment.
clean_tmp() can evict a reservation that belongs to a still-running transaction if course creation takes longer than the 30s timeout, however unlikely that may be. In that case commit_id(id) returns false and the in-memory ID set can fall out of sync with the DB until restart/resync.
Not a merge blocker since the DB primary key still protects correctness, but it might be worth logging/resyncing when/if commit_id(id) fails.
|
The request to create a course should not be taking 30 seconds but yeah that can still happen. We can bump the time if needed but for now it should be fine. |
2 participants
there is an issue with how the course ids are being generated that is causing duplicates to be generated consistently thus preventing the creation of new courses for the site. I detailed more of this in
api/src/utils/id_generator.ts. for now this will keep track of created course ids and pre-existing ones so that we can relyably create new ones without having to hit the database all the time and prevent potential race conditions by having each request calculate it.fixes #155