Side B fixes a real correctness bug (RwLock guard held across a match arm, invitable to deadlock, in RoomCreate/RoomGrant handlers) plus flaky-test root causes (missing HTTP timeouts, pipe-buffer deadlock in test harness log capture), all concrete and lasting improvements to reliability. Side A is a large structural refactor that consolidates duplicate web_post.rs handlers into a single /ui dispatcher and unifies session resolution, which is a reasonable cleanup but mostly moves/renames existing logic without fixing any bug, and its size is largely churn (route removal, JSON templating changes) rather than a necessary fix.
constitution · epochs · watch · epoch 3
comparison
c_effff18688f9 (tommy-mor) vs c_9608dc0d38ab (tommy-mor)
download prompt · raw event · cmp_51729252864a11
council reasoning
B fixes real tokio RwLock deadlocks in RoomCreate/RoomGrant by dropping read guards before nested lock acquires, plus complementary test infra (log-file pipes, HTTP timeouts) that prevents related hangs—correctness that keeps RPC usable. A is a large but mostly consolidating refactor (delete web_post.rs, fold ingest/check/redact into POST /ui and form __rpc__), valuable design cleanup without the same defect-level impact.
Side A performs a substantial architectural consolidation by moving browser posting, checking, and redaction into a single POST /ui dispatch path, introducing a shared WebSession resolver to avoid repeated auth lookups, removing the separate web_post module, and updating HTML, routing, and integration tests to use the unified RPC-style interface. Side B fixes a real correctness issue by shortening RwLock read-guard lifetimes to avoid deadlocks in RoomCreate/RoomGrant and adds regression coverage, but its scope is limited to a specific locking bug whereas Side A delivers a broader, lasting simplification of the web API structure.
sides
A — c_effff18688f9 (tommy-mor)
message
[c3cbcaa7] refactor
diff preview
diff --git a/server/src/api/auth.rs b/server/src/api/auth.rs
index b3631b06153d52f88348fef927e7a024b7b85ad6..cd556eb89e9dd8203eba6c8969ffd144db5d329d 100644
--- a/server/src/api/auth.rs
+++ b/server/src/api/auth.rs
@@ -71,6 +71,24 @@ pub fn optional_principal(headers: &HeaderMap, jar: &CookieJar, reduced: &Reduce
verify_token(reduced, c.value()).ok()
}
+/// Browser session: principal + bearer token string (same shape as CLI session cookie).
+#[derive(Debug, Clone)]
+pub struct WebSession {
+ pub username: String,
+ pub bearer: String,
+}
+
+/// Resolve username and bearer together for `POST /ui` dispatch (one read of headers + jar).
+pub fn resolve_web_session(headers: &HeaderMap, jar: &CookieJar, reduced: &ReducerState) -> Option<WebSession> {
+ let username = optional_principal(headers, jar, reduced)?;
+ let bearer = headers
+ .get(header::AUTHORIZATION)
+ .and_then(|v| v.to_str().ok())
+ .and_then(|s| s.strip_prefix("Bearer ").map(|t| t.trim().to_string()))
+ .or_else(|| jar.get(SLUG_SESSION_COOKIE).map(|c| c.value().to_string()))?;
+ Some(WebSession { username, bearer })
+}
+
fn redirect_with_session_cookie(public_url: &str, path_and_query: &str, bearer: &str, jar: &CookieJar) -> Response {
let mut res = Response::builder()
.status(StatusCode::TEMPORARY_REDIRECT)
diff --git a/server/src/api/mod.rs b/server/src/api/mod.rs
index a986f706ea4b261cbaf004c02b4cf84184b41371..4e223a7460997c464706dca850281447bd754ed5 100644
--- a/server/src/api/mod.rs
+++ b/server/src/api/mod.rs
@@ -4,7 +4,6 @@ mod rpc;
mod stream;
mod validate;
mod ui_html;
-mod web_post;
pub use auth::{
get_join_invite,
@@ -19,7 +18,9 @@ pub use auth::{
get_web_login,
get_logout,
optional_principal,
+ resolve_web_session,
session_cookie_header_value,
+ WebSession,
SLUG_SESSION_COOKIE,
};
@@ -35,7 +36,6 @@ pub use stream::{get_html_stream, get_stream};
pub use validate::{normalize_room_and_thread, validate_ingest_document, ValidatedIngest};
pub use ui_html::post_ui_html;
-pub use web_post::{check_web_ingest, post_web_ingest, post_web_redact};
#[cfg(test)]
mod tests {
diff --git a/server/src/api/ui_html.rs b/server/src/api/ui_html.rs
index 2b40a72059981d558768f73d189b991f3448c257..497f8fdd222a8ec7c3d76b0695e0352e973ca3ba 100644
--- a/server/src/api/ui_html.rs
+++ b/server/src/api/ui_html.rs
@@ -1,25 +1,29 @@
-//! Single `POST /ui` entry for browser [`crate::html::ui_action::HtmlUiAction`] (JSON in `__rpc__` + holes).
+//! Single `POST /ui` entry: parse `__rpc__` → [`HtmlUiAction`], resolve [`WebSession`] once, dispatch.
use axum::{
- body::Body,
+ body,
extract::State,
- http::{header, HeaderMap, StatusCode},
+ http::{header, HeaderMap, HeaderValue, StatusCode},
response::{IntoResponse, Response},
Form,
};
use axum_extra::extract::cookie::CookieJar;
+use slug_types::{RpcBatch, RpcBatchResponse, RpcCommand, RpcResult};
use std::collections::HashMap;
use crate::{
api::{
- auth::optional_principal,
- web_post::{run_check_web_ingest, run_post_web_ingest, run_post_web_redact, WebPostForm, WebRedactForm},
+ auth::{resolve_web_session, WebSession},
+ handle_rpc_batch,
+ rpc::{rpc_post_redact, rpc_post_with_bearer},
},
+ canonical_path::canonicalize_tag,
html::{
fragment_public_new_thread_form, fragment_room_new_thread_form, login_to_post_hint_markup,
- parse_html_ui_from_form, user_can_post_room, user_can_view_room, HtmlUiAction, JsBuilder,
- ThreadNav,
+ parse_html_ui_from_form, thread_feed_html, thread_feed_html_for_room, thread_feed_region_markup,
+ user_can_post_room, user_can_view_room, HtmlUiAction, JsBuilder, ThreadNav,
},
+ reducer::{scope_from_room_wire, ScopeId},
state::AppState,
};
@@ -34,6 +38,19 @@ pub async fn post_ui_html(
Err(e) => return ui_js_warn(&e.to_string()).into_response(),
};
+ let reduced = state.reduced.read().await;
+ let session = resolve_web_session(&headers, &jar, &reduced);
+ drop(reduced);
+
+ dispatch_ui_action(&state, session.as_ref(), action).await
+}
+
+/// All UI command logic: HTTP extractors stop above; this only sees [`AppState`], session, and [`HtmlUiAction`].
+async fn dispatch_ui_action(
+ state: &AppState,
+ session: Option<&WebSession>,
+ action: HtmlUiAction,
+) -> Response {
match action {
HtmlUiAction::PostIngest {
room,
@@ -42,47 +59,87 @@ pub async fn post_ui_html(
error_target,
form_id,
} => {
- run_post_web_ingest(
- &state,
- &headers,
- &jar,
- WebPostForm {
- room,
- thread_tag,
- text,
- error_target,
- form_id,
- },
- )
- .await
+ let Some(session) = session else {
+ return js_redirect("/login").into_response();
+ };
+ let room = room.trim().to_string();
+ let thread_tag = thread_tag.trim().to_string();
+ if text.trim().is_empty() {
+ return form_js_error(
+ error_target.as_ref(),
+ "empty post",
+ "Write something in the text area (DSL / prose).",
+ )
+ .into_response();
+ }
+ match rpc_post_with_bearer(state, &session.bearer, room.clone(), thread_tag.clone(), text).await {
+ Ok(RpcResult::PostOk { .. }) => {
+ post_success_response(
+ state,
+ &room,
+ &thread_tag,
+ error_target.as_ref(),
+ form_id.as_ref(),
+ Some(session.username.as_str()),
+ )
+ .await
+ .into_response()
+ }
+ Ok(_) => form_js_error(
+ error_target.as_ref(),
+ "unexpected response",
+ "Post did not return PostOk.",
+ )
+ .into_response(),
+ Err((msg, hint)) => form_js_error(error_target.as_ref(), &msg, hint.as_deref().unwrap_or("")).into_response(),
+ }
}
HtmlUiAction::CheckIngest {
room,
thread_tag,
text,
error_target,
- form_id,
+ form_id: _,
} => {
- run_check_web_ingest(
- &state,
- &headers,
- &jar,
- WebPostForm {
- room,
- thread_tag,
- text,
- error_target,
- form_id,
- },
- )
- .await
+ let Some(session) = session else {
+ return js_redirect("/login").into_response();
+ };
+ let room = room.trim().to_string();
+ let thread_tag = canonicalize_tag(&thread_tag);
+ if thread_tag.is_empty() {
+ return form_js_error(
+ error_target.as_ref(),
+ "missing thread tag",
+ "Set a thread tag before posting.",
+ )
+ .into_response();
+ }
+ if text.trim().is_empty() {
+ return js_clear_errors(&form_error_target(error_target.as_ref())).into_response();
+ }
+ match rpc_check_with_bearer(state, &session.bearer, room, text.clone()).await {
+ Ok(RpcResult::CheckOk { .. }) => js_clear_errors(&form_error_target(error_target.as_ref())).into_response(),
+ Ok(_) => form_js_error(error_target.as_ref(), "unexpected response", "Check did not return CheckOk.").into_response(),
+ Err((msg, hint)) => form_js_error(error_target.as_ref(), &msg, hint.as_deref().unwrap_or("")).into_response(),
+ }
}
HtmlUiAction::RedactPost { post_id } => {
- run_post_web_redact(&state, &headers, &jar, WebRedactForm { post_id }).await
+ let Some(session) = session else {
+ return js_redirect("/login").into_response();
+ };
+ let h = headers_from_bearer(&session.bearer);
+ match rpc_post_redact(state, &h, post_id).await {
+ Ok(RpcResult::RedactPostOk {}) => redact_success_response(state).await.into_response(),
+ Ok(_) => (StatusCode::BAD_REQUEST, "unexpected response").into_response(),
+ Err((msg, hint)) => {
+ let detail = hint.as_deref().unwrap_or("");
+ js_error("#errors", &msg, detail).into_response()
+ }
+ }
}
HtmlUiAction::ExpandPublicNewThreadForm => {
let reduced = state.reduced.read().await;
- let user = optional_principal(&headers, &jar, &reduced);
+ let user = session.map(|s| s.username.as_str());
drop(reduced);
let markup = if user.is_some() {
fragment_public_new_thread_form(true)
@@ -99,18 +156,18 @@ pub async fn post_ui_html(
return ui_js_warn("missing room").into_response();
}
let reduced = state.reduced.read().await;
- let user = optional_principal(&headers, &jar, &reduced);
+ let user = session.map(|s| s.username.as_str());
if !reduced.rooms.contains(&room_wire) {
drop(reduced);
return ui_js_warn("room not found").into_response();
}
- if !user_can_view_room(&reduced, &room_wire, user.as_deref()) {
+ if !user_can_view_room(&reduced, &room_wire, user) {
drop(reduced);
return ui_js_warn("forbidden").into_response();
}
- let can_post = user
+ let can_post = session
.as_ref()
- .map(|u| user_can_post_room(&reduced, &room_wire, u))
+ .map(|s| user_can_post_room(&reduced, &room_wire, &s.username))
.unwrap_or(false);
drop(reduced);
let Some(nav) = ThreadNav::from_room_id(&room_wire) else {
@@ -128,12 +185,195 @@ pub async fn post_ui_html(
}
}
+fn headers_from_bearer(bearer: &str) -> HeaderMap {
+ let mut headers = HeaderMap::new();
+ if let Ok(hv) = HeaderValue::from_str(&format!("Bearer {bearer}")) {
+ headers.insert(header::AUTHORIZATION, hv);
+ }
+ headers
+}
+
+fn post_redirect_location(room: &str, thread_tag: &str) -> String {
+ let tag = canonicalize_tag(thread_tag);
+ if room.trim() == "public" {
+ format!("/t/{tag}")
+ } else {
+ let room = room.trim();
+ let Some((a, b)) = room.split_once('/') else {
+ return "/".to_string();
+ };
+ format!("/r/{a}/{b}/t/{tag}")
+ }
+}
+
+fn js_quote(s: &str) -> String {
+ serde_json::to_string(s).expect("js string escaping must succeed")
+}
+
+fn js_redirect(to: &str) -> Response {
+ let js = format!("window.location = {};", js_quote(to));
+ Response::builder()
+ .status(StatusCode::OK)
+ .header(header::CONTENT_TYPE, "text/javascript; charset=utf-8")
+ .body(axum::body::Body::from(js))
+ .unwrap()
+}
+
+fn js_error(error_target: &str, title: &str, detail: &str) -> Response {
+ let markup = maud::html! {
+ div id=(error_target.trim_start_matches('#')) {
+ p class="auth-error" { (title) }
+ @if !detail.is_empty() {
+ pre class="muted" { (detail) }
+ }
+ }
+ };
+ JsBuilder::new()
+ .morph_selector(error_targe
… preview truncated; 33,445 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.