Data stops getting stored to session #2393
Comments
|
I have added a video to show what is happening for me at the moment when I use the latest version of Chrome and the GOV.UK Prototype Kit: https://www.loom.com/share/7ac82f624a3141aba433d9d309bd352d?sid=e8c845f2-0a70-400e-92f3-f93401d7d81d |
|
Operating system: macOS Sonoma |
|
Loom video showing my issue with data not being passes through pages. The applicant name and organisation name are inserted into subsequent questions to add context, the applicant name isn't being passed through. Also the details captured in the questions are not being displayed on the Check your answers page. https://www.loom.com/share/6b33464f6bc346c38b9dfea4835e7779?sid=bf532f85-42e8-41ec-a259-2477d4352b08 |
|
Operating system: macOS Catalina 10.15.7 |
|
Clearing .tmp/sessions via the Terminal fixed the issue for me. Entered:
in the VS Code terminal. Then restarted the app and it's working again. |
|
I am having the same issue that everyone else is. It affects prototypes running locally and in Heroku. I've not been able to reproduce the error reliably. It happens randomly when moving from one page, through a route to another page. It forgets everything stored in session and the session object is no longer able to be read from or written too. Given that clearing all the JSON files in .tmp/sessions fixes the issue- I think that something in there is the cause of the issue. I would go through these and check but given that a new JSON session file is created on every page load, this would take a prohibitive amount of time. Operating system: MacOS 14.3.1 (23D60) |
|
@HarryMtds thanks, does it happen in other browsers? |
|
Can't seem to replicate it in Safari. I have noticed an interesting difference in behaviour between Chrome and Safari. Chrome:
Safari:
Edit:On further inspection, chrome generates a new session file on every page load, whereas Safari does not |
|
@HarryMtds super helpful thanks! |
36degrees
moved this from Backlog 🏃🏼♀️
to In progress 📝
in GOV.UK Design System cycle board
|
I've been trying to get to the bottom of what's going on here… a few observations so far…
|
|
the timing of the issue seems to point to the manifest being a possible cause - we could try asking people who see this issue to try something like this in their |
We store session data in the filesystem using our own session `FileStore` class. Each session is stored as a separate JSON file inside `.tmp/sessions` with an expiry date and the data for session.
As part of the `set` method for the FileStore class, we have some logic which tries to clean up expired sessions from the filesystem.
This loops through all of the files in the session store directory and checks each one in turn to see if the expiry date in the JSON is in the past. If it is, it calls `this.destroy` passing the `storedSessionId`. This happens asynchronously – we map over the files in the directory to create an array of promises, each of which awaits a call to the filesystem to read the JSON.
Unfortunately, `storedSessionId` is scoped to the outer `set` method and is therefore affected by the other promises that are running concurrently.
Because of this, when there are expired sessions in the session store, it’s possible for non-expired sessions to be destroyed instead, depending on the value of `storedSessionId` at the time destroy is called.
Furthermore, because this leaves the expired session in place, further attempts to set data are likely to continue cleaning up the non-expired sessions.
You can observe this (and verify that this fixes the issue) by:
1. Adding logging just before the call to this.destroy on line 23:
```
console.log(`Destroying expired session ${storedSessionId}`)
```
2. Clearing all session data by deleting `.tmp/session`
3. Generating multiple non-expired sessions by refreshing the Prototype Kit multiple times [^1]
4. Manually updating the expiry date in the JSON for one of the sessions to be in the past [^2]
5. Refreshing the kit and observing that a session with a different ID is destroyed
Fix this by correctly scoping the `storedSessionId` variable to the asynchronous function, preventing it being altered by other promises resolving at the same time.
Due to the limited support we are currently able to provide for the kit, I have made the smallest possible change I believe fixes the bug and not done any further refactoring.
Also, whilst I believe it would be beneficial to have tests that cover this code (including this specific bug) I have not done this as it’s non-trivial and involves mocking the file system.
Closes #2393.
[1]: This works because the request for the new manifest added in GOV.UK Frontend v5.0 (served at `/plugin-assets/govuk-frontend/dist/govuk/assets/manifest.json` in the kit) is made without credentials (see the note on https://web.dev/articles/add-manifest) which results in an extra session being created (and never reused) every time the page is loaded. I think this also might have made this bug more likely to occur because of the sheer number of session files that get created.
[2]: In case it’s useful for testing, this jq command shows the expiry date for all current sessions:
```
jq -r -n 'inputs | "\(input_filename) \(.cookie.expires)"' .tmp/sessions/*.json | sort | head
```
Co-authored-by: Colin Rotherham <[email protected]>
36degrees
moved this from In progress 📝
to Needs review 🔍
in GOV.UK Design System cycle board
36degrees
moved this from Needs review 🔍
to Ready to release 🚀
in GOV.UK Design System cycle board
36degrees
moved this from Ready to release 🚀
to Done 🏁
in GOV.UK Design System cycle board
Description of the issue
We're getting a few reports that data stops getting stored to session sometimes.
Clearing
.tmp/sessionsseems to fixOther reports suggest this happens in Chrome and not other browsers
Steps to reproduce the issue
Actual vs expected behaviour
Environment (where applicable)
The text was updated successfully, but these errors were encountered: