On Fri, Nov 17, 2023 at 12:46:20AM +0100, Stefan Sperling wrote:
> I suspect the sharing of sock->pack_fds across cached open repositories
> during requests is the source of the "unexpecte EOF" errors that
> sometimes occur on my gotwebd instance.
> 
> This attempts to remove the cache again. The hard part is ensuring that
> the repository is closed once the request is done. Did I get this right?

There previous diff still file descriptor leaks, causing fd exhaustion
resulting in errors like "bad exit code from unprivileged process".
Presumbaly because a forked child ran out of available fd slots early on.
One sockets worker had 1024 open files, which is the hard limit.

New diff, with all the early returns in process_request changed to
close_repo gotos. Just in case I am also letting gotweb_load_got_path()
close open repositories on error.

-----------------------------------------------
 remove the gotwebd repository cache
 
 It only had 4 slots so was never quite useful, and sharing of sock->pack_fds
 across cached repositories seems problematic.
 
diff 8def94257de0c243b087788186b73585ffc24f25 6177f939367db2d2b53b702caacbd31130055546
commit - 8def94257de0c243b087788186b73585ffc24f25
commit + 6177f939367db2d2b53b702caacbd31130055546
blob - b35fa0bcfb79ea056b7d4831d3597333b7341e2a
blob + b1108c45d826da11c2d77f4d5c684f9c629ea7c8
--- gotwebd/config.c
+++ gotwebd/config.c
@@ -85,11 +85,6 @@ config_getserver(struct gotwebd *env, struct imsg *ims
 	IMSG_SIZE_CHECK(imsg, srv);
 
 	memcpy(srv, p, sizeof(*srv));
-	srv->cached_repos = calloc(GOTWEBD_REPO_CACHESIZE,
-	    sizeof(*srv->cached_repos));
-	if (srv->cached_repos == NULL)
-		fatal("%s: calloc", __func__);
-	srv->ncached_repos = 0;
 
 	/* log server info */
 	log_debug("%s: server=%s fcgi_socket=%s unix_socket=%s", __func__,
blob - 807b59726fb1bd60b655c7536a08f348058f1da3
blob + efe119f2a47137c986d6221afa89cb82410f23d1
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -215,7 +215,7 @@ gotweb_process_request(struct request *c)
 			goto err;
 	}
 
-	switch(qs->action) {
+	switch (qs->action) {
 	case BLAME:
 		error = got_get_repo_commits(c, 1);
 		if (error) {
@@ -223,9 +223,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_blame);
-		return;
+		goto close_repo;
 	case BLOB:
 		if (binary) {
 			struct gotweb_url url = {
@@ -239,13 +239,13 @@ gotweb_process_request(struct request *c)
 			};
 
 			gotweb_reply(c, 302, NULL, &url);
-			return;
+			goto close_repo;
 		}
 
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_blob);
-		return;
+		goto close_repo;
 	case BLOBRAW:
 		if (binary)
 			r = gotweb_reply_file(c, "application/octet-stream",
@@ -253,9 +253,9 @@ gotweb_process_request(struct request *c)
 		else
 			r = gotweb_reply(c, 200, "text/plain", NULL);
 		if (r == -1)
-			return;
+			goto close_repo;
 		if (template_flush(c->tp) == -1)
-			return;
+			goto close_repo;
 
 		for (;;) {
 			error = got_object_blob_read_block(&len, c->t->blob);
@@ -267,15 +267,15 @@ gotweb_process_request(struct request *c)
 			if (fcgi_write(c, buf, len) == -1)
 				break;
 		}
-		return;
+		goto close_repo;
 	case BRIEFS:
 		error = got_get_repo_commits(c, srv->max_commits_display);
 		if (error)
 			goto err;
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_briefs);
-		return;
+		goto close_repo;
 	case COMMITS:
 		error = got_get_repo_commits(c, srv->max_commits_display);
 		if (error) {
@@ -283,9 +283,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_commits);
-		return;
+		goto close_repo;
 	case DIFF:
 		error = got_get_repo_commits(c, 1);
 		if (error) {
@@ -298,9 +298,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_diff);
-		return;
+		goto close_repo;
 	case INDEX:
 		c->t->nrepos = scandir(srv->repos_path, &c->t->repos, NULL,
 		    alphasort);
@@ -311,18 +311,18 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_index);
-		return;
+		goto close_repo;
 	case RSS:
 		error = got_get_repo_tags(c, D_MAXSLCOMMDISP);
 		if (error)
 			goto err;
 		if (gotweb_reply_file(c, rss_ctype, repo_dir->name, ".rss")
 		    == -1)
-			return;
+			goto close_repo;
 		gotweb_render_rss(c->tp);
-		return;
+		goto close_repo;
 	case SUMMARY:
 		error = got_ref_list(&c->t->refs, c->t->repo, "refs/heads",
 		    got_ref_cmp_by_name, NULL);
@@ -343,9 +343,9 @@ gotweb_process_request(struct request *c)
 		}
 		qs->action = SUMMARY;
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_summary);
-		return;
+		goto close_repo;
 	case TAG:
 		error = got_get_repo_tags(c, 1);
 		if (error) {
@@ -358,9 +358,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_tag);
-		return;
+		goto close_repo;
 	case TAGS:
 		error = got_get_repo_tags(c, srv->max_commits_display);
 		if (error) {
@@ -368,9 +368,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_tags);
-		return;
+		goto close_repo;
 	case TREE:
 		error = got_get_repo_commits(c, 1);
 		if (error) {
@@ -378,9 +378,9 @@ gotweb_process_request(struct request *c)
 			goto err;
 		}
 		if (gotweb_reply(c, 200, "text/html", NULL) == -1)
-			return;
+			goto close_repo;
 		gotweb_render_page(c->tp, gotweb_render_tree);
-		return;
+		goto close_repo;
 	case ERR:
 	default:
 		error = got_error(GOT_ERR_BAD_QUERYSTRING);
@@ -389,8 +389,13 @@ gotweb_process_request(struct request *c)
 err:
 	c->t->error = error;
 	if (gotweb_reply(c, 400, "text/html", NULL) == -1)
-		return;
+		goto close_repo;
 	gotweb_render_page(c->tp, gotweb_render_error);
+close_repo:
+	if (c->t->repo) {
+		got_repo_close(c->t->repo);
+		c->t->repo = NULL;
+	}
 }
 
 struct server *
@@ -860,6 +865,10 @@ gotweb_render_index(struct template *tp)
 				    sd_dent[d_i]->d_name, error->msg);
 			gotweb_free_repo_dir(repo_dir);
 			repo_dir = NULL;
+			if (t->repo) {
+				got_repo_close(t->repo);
+				t->repo = NULL;
+			}
 			d_skipped++;
 			continue;
 		}
@@ -1104,75 +1113,13 @@ gotweb_render_absolute_url(struct request *c, struct g
 	return gotweb_render_url(c, url);
 }
 
-static struct got_repository *
-find_cached_repo(struct server *srv, const char *path)
-{
-	int i;
-
-	for (i = 0; i < srv->ncached_repos; i++) {
-		if (strcmp(srv->cached_repos[i].path, path) == 0)
-			return srv->cached_repos[i].repo;
-	}
-
-	return NULL;
-}
-
 static const struct got_error *
-cache_repo(struct got_repository **new, struct server *srv,
-    struct repo_dir *repo_dir, struct socket *sock)
-{
-	const struct got_error *error = NULL;
-	struct got_repository *repo;
-	struct cached_repo *cr;
-	int evicted = 0;
-
-	if (srv->ncached_repos >= GOTWEBD_REPO_CACHESIZE) {
-		cr = &srv->cached_repos[srv->ncached_repos - 1];
-		error = got_repo_close(cr->repo);
-		memset(cr, 0, sizeof(*cr));
-		srv->ncached_repos--;
-		if (error)
-			return error;
-		memmove(&srv->cached_repos[1], &srv->cached_repos[0],
-		    srv->ncached_repos * sizeof(srv->cached_repos[0]));
-		cr = &srv->cached_repos[0];
-		evicted = 1;
-	} else {
-		cr = &srv->cached_repos[srv->ncached_repos];
-	}
-
-	error = got_repo_open(&repo, repo_dir->path, NULL, sock->pack_fds);
-	if (error) {
-		if (evicted) {
-			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
-			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
-		}
-		return error;
-	}
-
-	if (strlcpy(cr->path, repo_dir->path, sizeof(cr->path))
-	    >= sizeof(cr->path)) {
-		if (evicted) {
-			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
-			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
-		}
-		return got_error(GOT_ERR_NO_SPACE);
-	}
-
-	cr->repo = repo;
-	srv->ncached_repos++;
-	*new = repo;
-	return NULL;
-}
-
-static const struct got_error *
 gotweb_load_got_path(struct request *c, struct repo_dir *repo_dir)
 {
 	const struct got_error *error = NULL;
 	struct socket *sock = c->sock;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
-	struct got_repository *repo = NULL;
 	DIR *dt;
 	char *dir_test;
 
@@ -1209,13 +1156,9 @@ done:
 		goto err;
 	}
 
-	repo = find_cached_repo(srv, repo_dir->path);
-	if (repo == NULL) {
-		error = cache_repo(&repo, srv, repo_dir, sock);
-		if (error)
-			goto err;
-	}
-	t->repo = repo;
+	error = got_repo_open(&t->repo, repo_dir->path, NULL, sock->pack_fds);
+	if (error)
+		goto done;
 	error = gotweb_get_repo_description(&repo_dir->description, srv,
 	    repo_dir->path, dirfd(dt));
 	if (error)
@@ -1234,6 +1177,10 @@ err:
 	free(dir_test);
 	if (dt != NULL && closedir(dt) == EOF && error == NULL)
 		error = got_error_from_errno("closedir");
+	if (error && t->repo) {
+		got_repo_close(t->repo);
+		t->repo = NULL;
+	}
 	return error;
 }
 
blob - a76ef44049655b82575dc5a398b4739eeb44efbc
blob + 71b34a0258f2a201a28b0745032caf69c7fcd5e6
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -47,7 +47,6 @@
 #define GOTWEBD_MAXNAME		 64
 #define GOTWEBD_MAXPORT		 6
 #define GOTWEBD_NUMPROC		 3
-#define GOTWEBD_REPO_CACHESIZE	 4
 #define GOTWEBD_SOCK_FILENO	 3
 
 #define PROC_MAX_INSTANCES	 32
@@ -290,18 +289,10 @@ struct address {
 };
 TAILQ_HEAD(addresslist, address);
 
-struct cached_repo {
-	char path[PATH_MAX];
-	struct got_repository *repo;
-};
-
 struct server {
 	TAILQ_ENTRY(server)	 entry;
 	struct addresslist	al;
 
-	struct cached_repo	*cached_repos;
-	int		 ncached_repos;
-
 	char		 name[GOTWEBD_MAXTEXT];
 
 	char		 repos_path[PATH_MAX];
blob - a16821af966e854627051ef7d1ddb100a024c3d1
blob + 4ac43e3ddee0f601ea33b3a80f934242683cdc4c
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -399,7 +399,6 @@ sockets_shutdown(void)
 {
 	struct server *srv, *tsrv;
 	struct socket *sock, *tsock;
-	int i;
 
 	sockets_purge(gotwebd_env);
 
@@ -411,11 +410,8 @@ sockets_shutdown(void)
 	}
 
 	/* clean servers */
-	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv) {
-		for (i = 0; i < srv->ncached_repos; i++)
-			got_repo_close(srv->cached_repos[i].repo);
+	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv)
 		free(srv);
-	}
 
 	free(gotwebd_env);
 }