Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CON-229] - Fix content caching #3322

Merged
merged 3 commits into from
Jun 27, 2022
Merged

[CON-229] - Fix content caching #3322

merged 3 commits into from
Jun 27, 2022

Conversation

vicky-g
Copy link
Contributor

@vicky-g vicky-g commented Jun 27, 2022

Description

We only want to cache on 404, but for some reason, content was still cached. Turns out it is because we were setting the cache-control header to public, max-age=2592000 even if content was not servable (e.g. 404-ing). The keyword public implies that content can be cached on client or server side.

So, this PR is to only set the the download/cache headers if content is definitely served.

Tests

Before, if the ipfs/<cid> route was getting a 404, it would result in a cache miss, then a cache hit.

After, all 404s will only result in a cache miss.

Monitoring - How will this change be monitored? Are there sufficient logs / alerts?

We will still monitor nginx logs in the logs file, and can observe the x-cache-status header

Copy link
Contributor

@SidSethi SidSethi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great find!! one Q

i think we'll need to make same change to dirCID route right? but can test this on staging first before making that change

creator-node/src/routes/files.js Show resolved Hide resolved
@vicky-g
Copy link
Contributor Author

vicky-g commented Jun 27, 2022

great find!! one Q

i think we'll need to make same change to dirCID route right? but can test this on staging first before making that change

actually, this is for the DIR scenario bc it sets the cache-control header, and then if the flow errs, the cache-control header is removed

  // Set the CID cache-control so that client cache the response for 30 days
  res.setHeader('cache-control', 'public, max-age=2592000, immutable')

  // Attempt to stream file to client
  try {
    req.logger.info(`Retrieving ${storagePath} directly from filesystem`)
    return await streamFromFileSystem(req, res, storagePath)
  } catch (e) {
    req.logger.info(`Failed to retrieve ${storagePath} from FS`)
  }

  // Attempt to find and stream CID from other content nodes in the network
  try {
    // CID is the file CID, parse it from the storagePath
    const CID = storagePath.split('/').slice(-1).join('')
    const libs = req.app.get('audiusLibs')
    await findCIDInNetwork(storagePath, CID, req.logger, libs)
    return await streamFromFileSystem(req, res, storagePath)
  } catch (e) {
    req.logger.error(
      `Error calling findCIDInNetwork for path ${storagePath}`,
      e
    )
    // Unset the cache-control header so that a bad response is not cached
    res.removeHeader('cache-control')
    return sendResponse(req, res, errorResponseServerError(e.message))
  }

@SidSethi
Copy link
Contributor

great find!! one Q
i think we'll need to make same change to dirCID route right? but can test this on staging first before making that change

actually, this is for the DIR scenario bc it sets the cache-control header, and then if the flow errs, the cache-control header is removed

  // Set the CID cache-control so that client cache the response for 30 days
  res.setHeader('cache-control', 'public, max-age=2592000, immutable')

  // Attempt to stream file to client
  try {
    req.logger.info(`Retrieving ${storagePath} directly from filesystem`)
    return await streamFromFileSystem(req, res, storagePath)
  } catch (e) {
    req.logger.info(`Failed to retrieve ${storagePath} from FS`)
  }

  // Attempt to find and stream CID from other content nodes in the network
  try {
    // CID is the file CID, parse it from the storagePath
    const CID = storagePath.split('/').slice(-1).join('')
    const libs = req.app.get('audiusLibs')
    await findCIDInNetwork(storagePath, CID, req.logger, libs)
    return await streamFromFileSystem(req, res, storagePath)
  } catch (e) {
    req.logger.error(
      `Error calling findCIDInNetwork for path ${storagePath}`,
      e
    )
    // Unset the cache-control header so that a bad response is not cached
    res.removeHeader('cache-control')
    return sendResponse(req, res, errorResponseServerError(e.message))
  }

nvm, i see this is already done

@vicky-g vicky-g merged commit 77cb0ce into master Jun 27, 2022
@vicky-g vicky-g deleted the vg-fix-caching-2 branch June 27, 2022 23:07
SidSethi pushed a commit that referenced this pull request Jun 30, 2022
* bump inactive timeout

* set headers only if about to stream file

* remove change
SidSethi pushed a commit that referenced this pull request Jun 30, 2022
* bump inactive timeout

* set headers only if about to stream file

* remove change
SidSethi pushed a commit that referenced this pull request Jun 30, 2022
* bump inactive timeout

* set headers only if about to stream file

* remove change
sliptype added a commit that referenced this pull request Sep 10, 2023
sliptype pushed a commit that referenced this pull request Sep 10, 2023
[eaaf509] [C-2565] Improve mobile app-redirect-popover experience (#3344) Dylan Jeffers
[1755765] Update sdk to 2.0.3-beta.18 (#3345) Sebastian Klingler
[571c991] [CON-676] Support storage v2 playlists+albums, and bump libs version to 2.0.3-beta.17 (#3314) Theo Ilie
[3cca778] [C-2577] Fix mobile web social buttons by removing pull-to-refresh (#3343) Dylan Jeffers
[0915ff4] [C-2519] Ignore TOS deeplink (#3341) Dylan Jeffers
[39fda1a] Feature flag gates to send metadata directly through chain (#3311) Michelle Brier
[80278ce] Upgrade sdk to beta.16, update install script (#3339) Dylan Jeffers
[7c3aaa5] [C-2451] Fix infinite buffering in audio player (#3336) Sebastian Klingler
[be68f2e] Prevent fetchBlockees/blockers when chat disabled (#3338) Dylan Jeffers
[779cfa5] [C-2568] Silence player/error when audio is loaded (#3330) Sebastian Klingler
[869504c] HOTFIX track notification navigation (#3335) Dylan Jeffers
[f30507c] Dismiss keyboard on mobile chat screen top right press (#3333) Reed
[ad7938c] DMs: Don't increment unread if it's your own message (#3331) Marcus Pasell
[565f182] Fix keyboard occluding textinput in mobile chat screen (#3332) Reed
[a82cf59] DMs: Fix unread indicator not going away (#3329) Marcus Pasell
[e47f323] Mobile chat housekeeping (#3288) Reed
[47a5e84] [PAY-1207] Use Followers instead of Mutuals on compose chat modal (#3323) Marcus Pasell
[03c587d] [C-2393] Cache pods in CI (#3317) Sebastian Klingler
[22f4d46] Remove identity subscriptions reads/writes (#3005) Michelle Brier
[b9ddae1] [C-2566] Prevent player error on skip (#3327) Dylan Jeffers
[ca246f9] DMs: Fix store bug sending messages (#3326) Marcus Pasell
[309af58] Update fetchPermission condition (#3324) Dylan Jeffers
[a8e301e] [PAY-1199] Mobile inbox settings functional (#3290) Reed
[adc7854] [C-2563] Fix demo build (#3322) Sebastian Klingler
[d703f6e] Fix search-results saga error due to yield* (#3321) Dylan Jeffers
[d940ae6] Fix internal/external audius urls (#3320) Dylan Jeffers
[96cd42f] [C-2560] Improve checking for new notifs (#3316) Dylan Jeffers
[3130546] I broke main and I'm sorry (#3319) Marcus Pasell
[a3062fe] Remove dependency on `download.cid` for determining download eligibility (#3307) Michael Piazza
[fe04046] [PAY-901] hotfix: Use SPL amounts for BuyAudio flow (#3318) Marcus Pasell
[1c94f06] Fix mobile prem content track headers (#3315) Reed
[f87cb04] [PAY-1108] Chats Screen avoids play bar (#3295) Reed
[c5450a0] Revert "[PAY-901] hotfix: TransactionDetailsModal now expects wei amounts" (#3313) Marcus Pasell
[e3ca9e3] Fix stylelint (#3312) Raymond Jacobson
[2031f1a] Debounce reaction for longer on web (#3305) Raymond Jacobson
[492fa67] Notification settings improvements (#3264) Michelle Brier
[59710d6] Rename sdk_v2 to sdk_discovery_node_selector (#3310) Dylan Jeffers
[8519910] [PLAT-863] Fix reaction endpoint when sdk-v2 (#3309) Dylan Jeffers
[00ba722] Fix index issue with constructUrl (#3308) Dylan Jeffers
[b0b32ff] [PLAT-904] Fix duplicate notifications (#3304) Dylan Jeffers
[14c7814] Pass metadata to libs for addTracksToChainAndCnode (#3285) Michelle Brier
[75456fd] Add search multimap (#3306) Raymond Jacobson
[f8f5f02] [C-2282] Show tracks tab on own profile if all tracks hidden (#3294) Andrew Mendelsohn
[49fc834] [PAY-901] hotfix: TransactionDetailsModal now expects wei amounts (#3297) Marcus Pasell
[f1366ac] Fix eager fetch (#3302) Raymond Jacobson
[4b639ba] [C-2542] Update delete playlist confirmation drawer to conform to new designs (#3303) Kyle Shanks
[1ac5ec2] Fix taste maker copy (#3301) Isaac Solo
[de48e11] [PAY-1178] Fix new messages on new/unloaded chats (#3298) Marcus Pasell
[ea6ed55] Redirect if not allowed ai (#3300) Raymond Jacobson
[985a05c] Fix ai attrib upload (#3299) Raymond Jacobson
[a586435] [C-2554] Add AI Attribution (#3296) Dylan Jeffers
[64dde7f] [CON-674] Make track edit use v2 flow for storage v2 tracks (#3289) Theo Ilie
[d38d483] [C-2543, C-2544] Initial updates to mobile playlist management (#3287) Kyle Shanks
[ec9dc0e] Fix devnet in wallet authorize for saga phone (#3291) Raymond Jacobson
@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants