FIX blank internal pages (#164) (#292)

Hello 👋

since it affected my deployment of the pages server I started to look into the problem of the blank pages and think I found a solution for it:

1. There is no check if the file response is empty, neither in cache retrieval nor in writing of a cache. Also the provided method for checking for empty responses had a bug.
2. I identified the redirect response to be the issue here. There is a cache write with the full cache key (e. g. rawContent/user/repo|branch|route/index.html) happening in the handling of the redirect response. But the written body here is empty. In the triggered request from the redirect response the server then finds a cache item to the key and serves the empty body. A quick fix is the check for empty file responses mentioned in 1.
3. The decision to redirect the user comes quite far down in the upstream function. Before that happens a lot of stuff that may not be important since after the redirect response comes a new request anyway. Also, I suspect that this causes the caching problem because there is a request to the forge server and its error handling with some recursions happening before. I propose to move two of the redirects before "Preparing"
4. The recursion in the upstream function makes it difficult to understand what is actually happening. I added some more logging to have an easier time with that.
5. I changed the default behaviour to append a trailing slash to the path to true. In my tested scenarios it happened anyway. This way there is no recursion happening before the redirect.

I am not developing in go frequently and rarely contribute to open source -> so feedback of all kind is appreciated

closes #164

Reviewed-on: https://codeberg.org/Codeberg/pages-server/pulls/292
Reviewed-by: 6543 <6543@obermui.de>
Reviewed-by: crapStone <codeberg@crapstone.dev>
Co-authored-by: Hoernschen <julian.hoernschemeyer@mailbox.org>
Co-committed-by: Hoernschen <julian.hoernschemeyer@mailbox.org>

server/gitea/cache.go

@@ -39,7 +39,7 @@ type FileResponse struct {
 }
 
 func (f FileResponse) IsEmpty() bool {
-	return len(f.Body) != 0
+	return len(f.Body) == 0
 }
 
 func (f FileResponse) createHttpResponse(cacheKey string) (header http.Header, statusCode int) {
@@ -72,13 +72,14 @@ type BranchTimestamp struct {
 type writeCacheReader struct {
 	originalReader io.ReadCloser
 	buffer         *bytes.Buffer
-	rileResponse   *FileResponse
+	fileResponse   *FileResponse
 	cacheKey       string
 	cache          cache.ICache
 	hasError       bool
 }
 
 func (t *writeCacheReader) Read(p []byte) (n int, err error) {
+	log.Trace().Msgf("[cache] read %q", t.cacheKey)
 	n, err = t.originalReader.Read(p)
 	if err != nil && err != io.EOF {
 		log.Trace().Err(err).Msgf("[cache] original reader for %q has returned an error", t.cacheKey)
@@ -90,12 +91,20 @@ func (t *writeCacheReader) Read(p []byte) (n int, err error) {
 }
 
 func (t *writeCacheReader) Close() error {
-	if !t.hasError {
+	doWrite := !t.hasError
-		fc := *t.rileResponse
+	fc := *t.fileResponse
-		fc.Body = t.buffer.Bytes()
+	fc.Body = t.buffer.Bytes()
-		_ = t.cache.Set(t.cacheKey, fc, fileCacheTimeout)
+	if fc.IsEmpty() {
+		log.Trace().Msg("[cache] file response is empty")
+		doWrite = false
 	}
-	log.Trace().Msgf("cacheReader for %q saved=%t closed", t.cacheKey, !t.hasError)
+	if doWrite {
+		err := t.cache.Set(t.cacheKey, fc, fileCacheTimeout)
+		if err != nil {
+			log.Trace().Err(err).Msgf("[cache] writer for %q has returned an error", t.cacheKey)
+		}
+	}
+	log.Trace().Msgf("cacheReader for %q saved=%t closed", t.cacheKey, doWrite)
 	return t.originalReader.Close()
 }
 
@@ -108,7 +117,7 @@ func (f FileResponse) CreateCacheReader(r io.ReadCloser, cache cache.ICache, cac
 	return &writeCacheReader{
 		originalReader: r,
 		buffer:         bytes.NewBuffer(make([]byte, 0)),
-		rileResponse:   &f,
+		fileResponse:   &f,
 		cache:          cache,
 		cacheKey:       cacheKey,
 	}

server/gitea/client.go

@@ -112,26 +112,27 @@ func (client *Client) GiteaRawContent(targetOwner, targetRepo, ref, resource str
 func (client *Client) ServeRawContent(targetOwner, targetRepo, ref, resource string) (io.ReadCloser, http.Header, int, error) {
 	cacheKey := fmt.Sprintf("%s/%s/%s|%s|%s", rawContentCacheKeyPrefix, targetOwner, targetRepo, ref, resource)
 	log := log.With().Str("cache_key", cacheKey).Logger()
-
+	log.Trace().Msg("try file in cache")
 	// handle if cache entry exist
 	if cache, ok := client.responseCache.Get(cacheKey); ok {
 		cache := cache.(FileResponse)
 		cachedHeader, cachedStatusCode := cache.createHttpResponse(cacheKey)
 		// TODO: check against some timestamp mismatch?!?
 		if cache.Exists {
+			log.Debug().Msg("[cache] exists")
 			if cache.IsSymlink {
 				linkDest := string(cache.Body)
 				log.Debug().Msgf("[cache] follow symlink from %q to %q", resource, linkDest)
 				return client.ServeRawContent(targetOwner, targetRepo, ref, linkDest)
-			} else {
+			} else if !cache.IsEmpty() {
-				log.Debug().Msg("[cache] return bytes")
+				log.Debug().Msgf("[cache] return %d bytes", len(cache.Body))
 				return io.NopCloser(bytes.NewReader(cache.Body)), cachedHeader, cachedStatusCode, nil
+			} else if cache.IsEmpty() {
+				log.Debug().Msg("[cache] is empty")
 			}
-		} else {
-			return nil, cachedHeader, cachedStatusCode, ErrorNotFound
 		}
 	}
-
+	log.Trace().Msg("file not in cache")
 	// not in cache, open reader via gitea api
 	reader, resp, err := client.sdkClient.GetFileReader(targetOwner, targetRepo, ref, resource, client.supportLFS)
 	if resp != nil {
@@ -155,12 +156,14 @@ func (client *Client) ServeRawContent(targetOwner, targetRepo, ref, resource str
 					linkDest = path.Join(path.Dir(resource), linkDest)
 
 					// we store symlink not content to reduce duplicates in cache
-					if err := client.responseCache.Set(cacheKey, FileResponse{
+					fileResponse := FileResponse{
 						Exists:    true,
 						IsSymlink: true,
 						Body:      []byte(linkDest),
 						ETag:      resp.Header.Get(ETagHeader),
-					}, fileCacheTimeout); err != nil {
+					}
+					log.Trace().Msgf("file response has %d bytes", len(fileResponse.Body))
+					if err := client.responseCache.Set(cacheKey, fileResponse, fileCacheTimeout); err != nil {
 						log.Error().Err(err).Msg("[cache] error on cache write")
 					}
 

server/handler/handler.go

@@ -26,6 +26,7 @@ func Handler(
 	dnsLookupCache, canonicalDomainCache, redirectsCache cache.ICache,
 ) http.HandlerFunc {
 	return func(w http.ResponseWriter, req *http.Request) {
+		log.Debug().Msg("\n----------------------------------------------------------")
 		log := log.With().Strs("Handler", []string{req.Host, req.RequestURI}).Logger()
 		ctx := context.New(w, req)
 

server/upstream/upstream.go

@@ -56,6 +56,8 @@ type Options struct {
 func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redirectsCache cache.ICache) bool {
 	log := log.With().Strs("upstream", []string{o.TargetOwner, o.TargetRepo, o.TargetBranch, o.TargetPath}).Logger()
 
+	log.Debug().Msg("Start")
+
 	if o.TargetOwner == "" || o.TargetRepo == "" {
 		html.ReturnErrorPage(ctx, "forge client: either repo owner or name info is missing", http.StatusBadRequest)
 		return true
@@ -104,13 +106,16 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi
 
 	// Handle not found error
 	if err != nil && errors.Is(err, gitea.ErrorNotFound) {
+		log.Debug().Msg("Handling not found error")
 		// Get and match redirects
 		redirects := o.getRedirects(giteaClient, redirectsCache)
 		if o.matchRedirects(ctx, giteaClient, redirects, redirectsCache) {
+			log.Trace().Msg("redirect")
 			return true
 		}
 
 		if o.TryIndexPages {
+			log.Trace().Msg("try index page")
 			// copy the o struct & try if an index page exists
 			optionsForIndexPages := *o
 			optionsForIndexPages.TryIndexPages = false
@@ -121,6 +126,7 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi
 					return true
 				}
 			}
+			log.Trace().Msg("try html file with path name")
 			// compatibility fix for GitHub Pages (/example → /example.html)
 			optionsForIndexPages.appendTrailingSlash = false
 			optionsForIndexPages.redirectIfExists = strings.TrimSuffix(ctx.Path(), "/") + ".html"
@@ -130,8 +136,11 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi
 			}
 		}
 
+		log.Trace().Msg("not found")
+
 		ctx.StatusCode = http.StatusNotFound
 		if o.TryIndexPages {
+			log.Trace().Msg("try not found page")
 			// copy the o struct & try if a not found page exists
 			optionsForNotFoundPages := *o
 			optionsForNotFoundPages.TryIndexPages = false
@@ -142,6 +151,7 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi
 					return true
 				}
 			}
+			log.Trace().Msg("not found page missing")
 		}
 
 		return false
@@ -173,10 +183,12 @@ func (o *Options) Upstream(ctx *context.Context, giteaClient *gitea.Client, redi
 	// Append trailing slash if missing (for index files), and redirect to fix filenames in general
 	// o.appendTrailingSlash is only true when looking for index pages
 	if o.appendTrailingSlash && !strings.HasSuffix(ctx.Path(), "/") {
+		log.Trace().Msg("append trailing slash and redirect")
 		ctx.Redirect(ctx.Path()+"/", http.StatusTemporaryRedirect)
 		return true
 	}
 	if strings.HasSuffix(ctx.Path(), "/index.html") && !o.ServeRaw {
+		log.Trace().Msg("remove index.html from path and redirect")
 		ctx.Redirect(strings.TrimSuffix(ctx.Path(), "index.html"), http.StatusTemporaryRedirect)
 		return true
 	}