Side B fixes a real concurrency bug: nested RwLock reads inside a match previously held a guard across further read/write calls, which could deadlock tokio's non-reentrant lock — a lasting correctness fix backed by a new integration test, plus corrects wrong test assertions (@user vs stored username) and hardens test harness timeouts to prevent flaky hangs. Side A is mostly UI polish (vote counts, HUD unpin button, CSS) which is useful but cosmetic and lower-stakes compared to a deadlock fix.
constitution · epochs · watch · epoch 3
comparison
c_3ff71f7eaeda (tommy-mor) vs c_9608dc0d38ab (tommy-mor)
download prompt · raw event · cmp_900f5042340da9
council reasoning
B fixes a real tokio RwLock deadlock in RoomCreate/RoomGrant by dropping the principal-verify guard before nested read/write awaits, plus an integration test and test harness hardening (server log redirection to avoid pipe deadlocks, HTTP timeouts). A is useful garden UX (vote-count badges, HUD unpin form, CSS, browser coverage) but is polish on an existing flow, not a core correctness fix.
Side B fixes a substantive concurrency bug by ensuring `RwLock` read guards are dropped before later `read().await`/`write().await` calls in RPC handlers, preventing potential deadlocks during room creation and grants. It also adds an integration test covering private room creation and improves test robustness with logging and HTTP timeouts, whereas Side A mainly enhances the garden UI (vote counts, HUD unpin button, styling) with supporting tests rather than addressing a core correctness issue.
sides
A — c_3ff71f7eaeda (tommy-mor)
message
[30a67104] fixes
diff preview
diff --git a/agents.md b/agents.md
index a6a283716e09fcaba1fd690f4e877e0bbecda2c0..d9a924d2f77c444d9b112bbf37a480b963ace4f0 100644
--- a/agents.md
+++ b/agents.md
@@ -39,7 +39,7 @@ Strict **CSP** that blocks `eval` would break the current app. Other projects ma
- **`VoteComparePost`:** On success returns **`text/javascript`** that **morphs** **`#vote-compare-preview`** (new ingest card) and **`#vote-edge-history-region`** (recomputed edge list). Uses **`RpcResult::PostOk`**’s **`post_id`** / **`post_index`** for the card. **`__rpc__`** carries **`form_action: "/ui"`**; **`thread_tag`** and ratio fields come from the same form as **`$form`** holes.
-- **Garden pin / compare voting:** Cookie **`slug_garden_pin`** via **`set_garden_pin`**. Pairwise UI: **`GET /vote/compare?…`** / **`GET /r/:room_key/vote/compare?…`**. HUD: **`#slug-pin-hud`** when **`layout`** passes garden metadata on **`body`**.
+- **Garden pin / compare voting:** Cookie **`slug_garden_pin`** via **`set_garden_pin`**. Pairwise UI: **`GET /vote/compare?…`** / **`GET /r/:room_key/vote/compare?…`**. HUD: **`#slug-pin-hud`** when **`layout`** passes garden metadata on **`body`**; the label is **`POST /ui`** **`set_garden_pin`** **`clear:true`** (**`slug_ui.js`**), not a permalink to the item.
**Rule of thumb:** New **CLI or API** verbs → `RpcCommand`. New **in-page morph or form-driven** behavior that only makes sense in the browser → `HtmlUiAction`. If both need the same operation, implement the real work once (e.g. call shared RPC helpers from `post_ui_html`) and keep the wire shapes separate.
diff --git a/server/src/html/garden.rs b/server/src/html/garden.rs
index 8a46b1e92d38f2c390f88d48750d95d11388cd73..121d9498e8cb93d4d001dc1bbce23d74fbb958f5 100644
--- a/server/src/html/garden.rs
+++ b/server/src/html/garden.rs
@@ -16,7 +16,6 @@ use crate::{
canonical_path::{canonicalize_item, canonicalize_tag},
form_template::template_json_compact,
html::{
- forum::ingest_entry_markup,
ui_action::UI_RPC_FIELD,
user_can_post_room,
JsBuilder,
@@ -111,6 +110,23 @@ fn votes_for_edge(content: &ContentState, a: &ItemId, b: &ItemId) -> Vec<crate::
out
}
+/// Number of vote ingests recorded for this unordered pair in `content` (same scope as ranking).
+fn edge_vote_count_for_pair(content: &ContentState, a: &ItemId, b: &ItemId) -> usize {
+ let (lo, hi) = canonical_edge_items(a, b);
+ let lo_s = lo.as_str();
+ let hi_s = hi.as_str();
+ content
+ .item_votes
+ .get(&lo)
+ .into_iter()
+ .flat_map(|q| q.iter())
+ .filter(|v| {
+ (v.a.as_str() == lo_s && v.b.as_str() == hi_s)
+ || (v.a.as_str() == hi_s && v.b.as_str() == lo_s)
+ })
+ .count()
+}
+
fn vote_thread_tags_for_pair(content: &ContentState, a: &ItemId, b: &ItemId) -> Vec<String> {
let set: HashSet<String> = content
.item_threads
@@ -288,6 +304,7 @@ fn child_row_pin_or_vote(
nav: &ThreadNav,
row_item: &ItemId,
pinned_room_and_item: Option<&(String, ItemId)>,
+ scope_content: &ContentState,
next_path: &str,
) -> maud::Markup {
let pin_matches_scope = pinned_room_and_item
@@ -315,7 +332,16 @@ fn child_row_pin_or_vote(
@if pi == row_item {
span class="ont-garden-pinned-here" title="Pinned" aria-label="Pinned" { "📌" }
} @else {
- a class="ont-garden-vote-ico" href=(vote_compare_href(nav, pi, row_item, None)) title="Vote vs pinned" aria-label="Vote" { "⚖" }
+ @let nv = edge_vote_count_for_pair(scope_content, pi, row_item);
+ @let tip = format!(
+ "Compare and vote — {nv} pairwise vote{} in this scope for pinned vs this row",
+ if nv == 1 { "" } else { "s" },
+ );
+ @let aria = format!("Vote; {} pairwise {}", nv, if nv == 1 { "vote" } else { "votes" });
+ a class="ont-garden-vote-ico" href=(vote_compare_href(nav, pi, row_item, None)) title=(tip) aria-label=(aria) {
+ span class="ont-garden-vote-glyph" aria-hidden="true" { "⚖" }
+ span class="ont-garden-vote-count" { (format!("{}", nv)) }
+ }
}
} @else {
form method="POST" action="/ui" data-navigate="full" class="ont-pin-form ont-garden-pin-form" {
@@ -978,10 +1004,9 @@ async fn render_scope_view(
) -> axum::response::Response {
let scope = nav.scope();
let pin_ref = pinned_item_from_jar(&jar);
- let model = {
- let reduced = state.reduced.read().await;
- build_item_page_view_model(&reduced, &scope, browse.item())
- };
+ let reduced = state.reduced.read().await;
+ let model = build_item_page_view_model(&reduced, &scope, browse.item());
+ let scope_content = content_for_garden_view(&reduced, &scope);
let thread_href = |tag: &str| nav.thread_url(tag);
let external_empty_body = browse.is_external() && model.body.is_none();
let cli_path_arg = item_display_path(&model.item);
@@ -1108,7 +1133,7 @@ async fn render_scope_view(
@let item_url = item_href(r.item.as_str(), &nav);
@let score_str = format!("{:.3}", r.score);
li data-garden-item=(r.item.as_str()) {
- (child_row_pin_or_vote(&nav, &r.item, pin_ref.as_ref(), &next_for_pin))
+ (child_row_pin_or_vote(&nav, &r.item, pin_ref.as_ref(), scope_content, &next_for_pin))
a class="item-link" href=(item_url) { code { (item_display_path(r.item.as_str())) } }
span class="ont-rank-score" { (score_str) }
}
@@ -1124,7 +1149,7 @@ async fn render_scope_view(
ul class="ont-group-list" {
@for name in &model.child_rankings.unranked_items {
li data-garden-item=(name.as_str()) {
- (child_row_pin_or_vote(&nav, name, pin_ref.as_ref(), &next_for_pin))
+ (child_row_pin_or_vote(&nav, name, pin_ref.as_ref(), scope_content, &next_for_pin))
@let href = item_href(name.as_str(), &nav);
a class="item-link" href=(href) { code { (item_display_path(name.as_str())) } }
}
@@ -1363,6 +1388,33 @@ mod tests {
}));
}
+ #[test]
+ fn edge_vote_count_for_pair_matches_votes_for_edge_len() {
+ use super::{
+ content_for_garden_view, edge_vote_count_for_pair, votes_for_edge,
+ };
+ use crate::path_types::ItemId;
+ let mut reduced = ReducerState::default();
+ apply_ingest(
+ &mut reduced,
+ 1,
+ "@00000000-0000-0000-0000-000000000000:test:local/test\n\
+ ~/topic {root}\n\
+ ~/topic/a {alpha}\n\
+ ~/topic/b {beta}\n\
+ ~/topic/a 3:2 ~/topic/b {first vote}\n\
+ ~/topic/b 2:3 ~/topic/a {second vote}\n",
+ );
+ let content = content_for_garden_view(&reduced, &ScopeId::Public);
+ let a = ItemId::parse("~/topic/a").unwrap().normalized_storage();
+ let b = ItemId::parse("~/topic/b").unwrap().normalized_storage();
+ assert_eq!(
+ edge_vote_count_for_pair(content, &a, &b),
+ votes_for_edge(content, &a, &b).len()
+ );
+ assert_eq!(votes_for_edge(content, &a, &b).len(), 2);
+ }
+
#[test]
fn item_page_model_includes_body_and_unranked_without_votes() {
let mut reduced = ReducerState::default();
diff --git a/server/static/slug_ui.js b/server/static/slug_ui.js
index c0de1cddbba78227dfb80bfd41e7b855b3a42bc3..86f935f8dd998f3d6df016f33b1e47f9780782ea 100644
--- a/server/static/slug_ui.js
+++ b/server/static/slug_ui.js
@@ -170,15 +170,6 @@
return { room: raw.slice(0, i), item: raw.slice(i + 1) };
}
- function gardenItemHref(prefix, storageUrl) {
- var marker = 'https://slug.social/~/';
- if (storageUrl.indexOf(marker) === 0) {
- var tail = storageUrl.slice(marker.length);
- return prefix.replace(/\/$/, '') + (tail ? '/' + tail : '');
- }
- return storageUrl;
- }
-
function refreshPinHud() {
var hud = document.getElementById('slug-pin-hud');
if (!hud) return;
@@ -187,19 +178,37 @@
var pin = decodePinCookie();
hud.innerHTML = '';
if (!pin || !prefix || pin.room !== bodyRoom) return;
- var a = document.createElement('a');
- a.className = 'slug-pin-hud-link';
- a.href = gardenItemHref(prefix, pin.item);
- a.title = 'Pinned item';
+ var form = document.createElement('form');
+ form.method = 'POST';
+ form.action = '/ui';
+ form.setAttribute('data-navigate', 'full');
+ form.className = 'slug-pin-hud-form';
+ var rpc = document.createElement('input');
+ rpc.type = 'hidden';
+ rpc.name = '__rpc__';
+ rpc.value = JSON.stringify({
+ action: 'set_garden_pin',
+ clear: true,
+ room_wire: '',
+ next: window.location.pathname + window.location.search,
+ form_action: '/ui',
+ });
+ form.appendChild(rpc);
+ var btn = document.createElement('button');
+ btn.type = 'submit';
+ btn.className = 'slug-pin-hud-link slug-pin-hud-unpin-btn';
+ btn.title = 'Unpin — removes this item from the corner HUD';
+ btn.setAttribute('aria-label', 'Unpin pinned item');
var span = document.createElement('span');
span.className = 'slug-pin-hud-glyph';
span.setAttribute('aria-hidden', 'true');
span.textContent = '📌';
- a.appendChild(span);
+ btn.appendChild(span);
var label = pin.item.replace(/^https:\/\/slug\.social\/~\/?/, '~/');
if (label.length > 36) label = label.slice(0, 34) + '…';
- a.appendChild(document.createTextNode(' ' + label));
- hud.appendChild(a);
+ btn.appendChild(document.createTextNode(' ' + label));
+ form.appendChild(btn);
+ hud.appendChild(form);
}
refreshPinHud();
diff --git a/server/static/theme_default.css b/server/static/theme_default.css
index ec0fbe7acee0aa2802f978f14a9b0fc86e78c5b8..9178f0629cfb868348740e6dea1626dc6afb8345 100644
--- a/server/static/theme_default.css
+++ b/server/static/theme_default.css
@@ -799,6 +799,12 @@ details > summary::-webkit-details-marker { display: none; }
}
/* Pinned item HUD — bottom bar, same plane as spread */
+.slug-pin-hud-form {
+ display: inline;
+ margin: 0;
+ padding: 0;
+ border: none;
+}
#slug-pin-hud.slug-pin-hud {
margin-left: auto;
max-width: min(42vw, 280px);
@@ -808,6 +814,13 @@ details > summary::-webkit-details-marker { display: none; }
overflow: hidden;
text-overflow: ellipsis;
}
+.slug-pin-hud-link.slug-pin-hud-unpin-btn {
+ background: transparent;
+ border: none;
+ cursor: pointer;
+ font-size: inherit;
+ font-family: inherit;
+}
.slug-pin-hud-link {
color: var(--ui);
text-decoration: none;
@@ -815,7 +828,10 @@ details > summary::-webkit-details-marker { display: none; }
align-items: center;
gap: 4px;
}
-.slug-pin-hud-link:hover { color: var(--signal); }
+.slug-pin-hud-link:hover,
+.slug-pin-hud-unpin-btn:hover {
+ color: var(--signal);
+}
.slug-pin-hud-glyph { font-size: 13px; line-height: 1; }
/* Garden pin / vote controls */
@@ -890,10 +906,21 @@ span.ont-garden-pinned-here {
align-items: center;
justify-content: center;
}
+a.ont-garden-vote-ico {
+ gap: 4px;
+}
a.ont-garden-vote-ico:hover {
color: var(--signal);
background: var(--g4);
}
+.ont-garden-v
… preview truncated; 4,237 characters omittedB — c_9608dc0d38ab (tommy-mor)
message
[9ecc4e2e] nice
diff preview
diff --git a/server/src/api/rpc.rs b/server/src/api/rpc.rs
index 5675dcd2e28062acbe3c037b94b77c33adf32474..a18241f3ed7b4a248748fcefc799f9801ca62461 100644
--- a/server/src/api/rpc.rs
+++ b/server/src/api/rpc.rs
@@ -936,7 +936,15 @@ pub async fn handle_rpc_batch(
line_ok(RpcResult::ForumThreads(rpc_list_forum_threads(&reduced, &room)))
}
RpcCommand::RoomCreate { slug, visibility } => {
- match verify_bearer_principal(&headers, &*state.reduced.read().await) {
+ // Scope the first read so its guard drops before any nested `read().await` / `write().await`.
+ // A guard from `match verify(..., &*state.reduced.read().await)` would otherwise live for the
+ // whole `match` and deadlock here (tokio::sync::RwLock is not reentrant).
+ let principal = {
+ let reduced = state.reduced.read().await;
+ verify_bearer_principal(&headers, &*reduced)
+ };
+ match principal {
+ Err((_, m)) => line_err(m, None),
Ok(principal) => {
let slug = slug.trim().to_lowercase();
if slug.is_empty() || slug.len() > 64 {
@@ -994,7 +1002,6 @@ pub async fn handle_rpc_batch(
}
}
}
- Err((_, m)) => line_err(m, None),
}
}
RpcCommand::RoomGrant {
@@ -1002,17 +1009,28 @@ pub async fn handle_rpc_batch(
username,
capability,
} => {
- match verify_bearer_principal(&headers, &*state.reduced.read().await) {
+ let principal = {
+ let reduced = state.reduced.read().await;
+ verify_bearer_principal(&headers, &*reduced)
+ };
+ match principal {
Err((_, m)) => line_err(m, None),
Ok(principal) => {
- let reduced = state.reduced.read().await;
- if !reduced.user_has_cap(&room, &principal, ThreadCapability::Manage) {
+ let can_manage = {
+ let reduced = state.reduced.read().await;
+ reduced.user_has_cap(&room, &principal, ThreadCapability::Manage)
+ };
+ if !can_manage {
line_err("requires Manage capability", None)
} else {
match parse_username(&username) {
Err(msg) => line_err("invalid username", Some(msg)),
Ok(target) => {
- if !reduced.users_by_provider.values().any(|u| u == &target) {
+ let user_exists = {
+ let reduced = state.reduced.read().await;
+ reduced.users_by_provider.values().any(|u| u == &target)
+ };
+ if !user_exists {
line_err(format!("user @{target} not found"), None)
} else {
match parse_capability(&capability) {
diff --git a/server/tests/integration.rs b/server/tests/integration.rs
index 9162bd5bee84035e3908bfb9c8e201b7878ca339..b930120da09fe7d307f0411b84fb639fb8bd0b15 100644
--- a/server/tests/integration.rs
+++ b/server/tests/integration.rs
@@ -95,6 +95,23 @@ async fn test_healthz() {
assert_eq!(response.text().await.unwrap(), "ok");
}
+#[tokio::test]
+async fn test_room_create_private_rpc() {
+ let (addr, _tmp, _log, _handle) = create_test_server().await;
+ let client = reqwest::Client::new();
+ let batch = serde_json::json!([{
+ "RoomCreate": { "slug": "secret-project", "visibility": "private" }
+ }]);
+ let body = rpc_batch(&client, addr, Some(&test_bearer()), batch).await;
+ let line = &body["results"][0];
+ assert_eq!(line["ok"], true, "room create: {:?}", line);
+ let room_id = line["result"]["RoomCreated"]["room_id"].as_str().unwrap();
+ assert!(
+ room_id.contains("/secret-project"),
+ "expected room_id to contain slug, got {room_id}"
+ );
+}
+
#[tokio::test]
async fn test_index_page() {
// HTML routes are offline during the auth-v3 refactor.
diff --git a/test/auth.bb b/test/auth.bb
index 611e04f1806ef81d678a91f499597fe55f691dd7..a67cc1de763434176d2f68e419dd37a8cd328395 100644
--- a/test/auth.bb
+++ b/test/auth.bb
@@ -176,7 +176,7 @@
(assert! (= 200 (:status poll)) "pending-session poll returns 200")
(let [poll-json (json/parse-string (:body poll) true)]
(assert! (:complete poll-json) "pending session complete=true")
- (assert! (= "@bbuser" (:user poll-json)) "poll returns @bbuser")
+ (assert! (= "bbuser" (:user poll-json)) "poll returns stored username bbuser")
(assert! (clojure.string/starts-with? (:token poll-json) "slug_") "poll returns bearer token")
(println "\nwhoami…")
@@ -184,7 +184,7 @@
:headers {"Authorization" (str "Bearer " (:token poll-json))})]
(assert! (= 200 (:status who)) "whoami returns 200")
(let [who-json (json/parse-string (:body who) true)]
- (assert! (= "@bbuser" (:user who-json)) "whoami user is @bbuser"))))))
+ (assert! (= "bbuser" (:user who-json)) "whoami user is bbuser (stored form)"))))))
(println "\nCLI: identity start → OAuth → identity poll → whoami…")
(let [cli-home (str tmp-dir "/cli-home")
@@ -208,7 +208,7 @@
(str "identity poll exits 0 (stderr: " (:err poll-proc) ")"))
(let [poll-cli (json/parse-string (:out poll-proc) true)]
(assert! (= "complete" (:phase poll-cli)) "identity poll --json phase")
- (assert! (= "@cliuser" (:user poll-cli)) "CLI poll user")
+ (assert! (= "cliuser" (:user poll-cli)) "CLI poll user (stored form)")
(assert! (clojure.string/starts-with? (:token poll-cli) "slug_") "CLI poll token")
(let [token-path (str cli-home "/.config/slugsocial/token")]
(assert! (fs/exists? token-path) "token written under isolated HOME")
@@ -219,7 +219,7 @@
(assert! (zero? (:exit who-proc))
(str "whoami exits 0 (stderr: " (:err who-proc) ")"))
(let [who-cli (json/parse-string (:out who-proc) true)]
- (assert! (= "@cliuser" (:user who-cli)) "CLI whoami uses saved token"))))))))
+ (assert! (= "cliuser" (:user who-cli)) "CLI whoami uses saved token"))))))))
(finally
(when-some [s @!server] (common/kill-server s))
diff --git a/test/common.bb b/test/common.bb
index ebb16c615a8b40e8830b5d5765d1e935a45538d0..4ed450377cdbb020f5ff164a812dfbbfc23c0f47 100644
--- a/test/common.bb
+++ b/test/common.bb
@@ -78,9 +78,20 @@
(defn start-server
"Start the slugsocial-server binary with the given env map.
- Returns the babashka.process map."
- [server-bin env-map]
- (p/process [server-bin] {:out :inherit :err :inherit :env env-map}))
+ Returns the babashka.process map.
+
+ When `log-file` (string path) is provided, stdout and stderr are appended there
+ instead of inheriting the parent descriptors. Inheriting shared pipes while the
+ parent blocks on HTTP I/O can fill the pipe buffer and deadlock the server on log writes."
+ ([server-bin env-map]
+ (start-server server-bin env-map nil))
+ ([server-bin env-map log-file]
+ (p/process [server-bin]
+ (if log-file
+ ;; Two string paths (same file): babashka.process can deref the process cleanly.
+ ;; :err :out + ProcessBuilder$Redirect breaks stream copying in deref/kill-server.
+ {:env env-map :out log-file :err log-file}
+ {:out :inherit :err :inherit :env env-map}))))
(defn kill-server
"Forcibly kill a server process (babashka.process map) and wait for it to exit."
diff --git a/test/grants.bb b/test/grants.bb
index 7066c1f2a57f39a362052f8524206dccf37bb7b1..793ba216f2de76a77eb20a76d08403db07ff411b 100644
--- a/test/grants.bb
+++ b/test/grants.bb
@@ -35,13 +35,14 @@
(defn- http-client []
(-> (java.net.http.HttpClient/newBuilder)
(.followRedirects java.net.http.HttpClient$Redirect/ALWAYS)
+ (.connectTimeout (java.time.Duration/ofSeconds 15))
(.build)))
(defn- http-get [url & {:keys [headers]}]
(let [b (java.net.http.HttpRequest/newBuilder (java.net.URI/create url))]
(doseq [[k v] (or headers {})]
(.header b k v))
- (let [req (-> b (.GET) (.build))
+ (let [req (-> b (.timeout (java.time.Duration/ofSeconds 60)) (.GET) (.build))
resp (.send (http-client) req (java.net.http.HttpResponse$BodyHandlers/ofString))]
{:status (.statusCode resp) :body (.body resp)})))
@@ -52,6 +53,7 @@
(doseq [[k v] (or headers {})]
(.header b k v))
(let [req (-> b
+ (.timeout (java.time.Duration/ofSeconds 60))
(.POST (java.net.http.HttpRequest$BodyPublishers/ofString body))
(.build))
resp (.send (http-client) req (java.net.http.HttpResponse$BodyHandlers/ofString))]
@@ -67,6 +69,7 @@
b (java.net.http.HttpRequest/newBuilder (java.net.URI/create url))]
(.header b "Content-Type" "application/x-www-form-urlencoded")
(let [req (-> b
+ (.timeout (java.time.Duration/ofSeconds 60))
(.POST (java.net.http.HttpRequest$BodyPublishers/ofString pairs))
(.build))
resp (.send (http-client) req (java.net.http.HttpResponse$BodyHandlers/ofString))]
Hardlinks — judgments / attempts / prompt
judgments
attempts
Prompt text is loaded only by the download route.