a73x

fef0420c

Resolve an issue prefix when renewing or releasing a lease

a73x   2026-09-06 09:15

Commit message
Resolve an issue prefix when renewing or releasing a lease
src/lease.rs
Old New
@@ -155,7 +155,16 @@ pub fn unclaim(repo: &Repository, remote_name: &str, id: &str, json: bool) -> Re
155 if is_conflict { 155 if is_conflict {
156 return Err(conflict(&value, id, json)); 156 return Err(conflict(&value, id, json));
157 } 157 }
158 emit(&value, json, || format!("Released the claim on {}", id)); 158 emit(&value, json, || {
159 // "released" and "not-held" are both exit 0, and saying "released"
160 // for either is what hid a bug where a claim made by prefix could
161 // not be released by that prefix at all (2026-09-06).
162 if value["status"] == "not-held" {
163 format!("No claim on {} to release", short(repo, &value, id))
164 } else {
165 format!("Released the claim on {}", short(repo, &value, id))
166 }
167 });
159 Ok(()) 168 Ok(())
160 } 169 }
161 170
src/server/leases.rs
Old New
@@ -86,8 +86,13 @@ pub enum Renew {
86 86
87 #[derive(Debug, PartialEq)] 87 #[derive(Debug, PartialEq)]
88 pub enum Release { 88 pub enum Release {
89 /// Row deleted, or there was nothing to release (idempotent success). 89 /// The lease was held and is now free.
90 Released, 90 Released,
91 /// There was nothing to release. Still a success — a client retrying after
92 /// a dropped connection must not fail — but reported distinctly, because
93 /// answering "released" to a release that freed nothing is what hid the
94 /// prefix bug of 2026-09-06 for a whole demo.
95 NotHeld,
91 /// A different holder has a live lease; refuse. 96 /// A different holder has a live lease; refuse.
92 NotHolder { holder: String }, 97 NotHolder { holder: String },
93 } 98 }
@@ -109,6 +114,57 @@ pub fn repo_key(repos_dir: &Path, repo_path: &Path) -> String {
109 .to_string() 114 .to_string()
110 } 115 }
111 116
117 /// What an id or prefix names among a repository's lease rows.
118 #[derive(Debug, PartialEq)]
119 pub enum Resolved {
120 /// Exactly one row. Its full `issue_id`.
121 One(String),
122 /// No row — nothing is leased under that id.
123 None,
124 /// Several rows share the prefix; the caller must be more specific.
125 Ambiguous(usize),
126 }
127
128 /// Resolve an issue id or prefix against the rows this repository actually
129 /// holds leases for.
130 ///
131 /// `acquire` resolves a prefix against the repository's issue refs, because it
132 /// has to know the issue exists and is open. `renew` and `release` cannot: the
133 /// issue may have been closed, or its ref deleted, while a lease is still
134 /// held, and a lease must stay releasable either way. So they resolve against
135 /// the lease table, which is the only thing that can answer "which lease did
136 /// you mean".
137 ///
138 /// Without this, releasing by prefix silently touched a key no row used and
139 /// reported success, leaving the lease held until its TTL ran out — found by
140 /// running two real workers against a real server on 2026-09-06, not by any
141 /// test, because every test used a full id on both sides.
142 pub fn resolve(conn: &Connection, repo: &str, id_or_prefix: &str) -> rusqlite::Result<Resolved> {
143 // An exact hit wins without a scan, and cannot be ambiguous.
144 let exact: Option<String> = conn
145 .query_row(
146 "SELECT issue_id FROM leases WHERE repo = ?1 AND issue_id = ?2",
147 (repo, id_or_prefix),
148 |row| row.get(0),
149 )
150 .optional()?;
151 if let Some(id) = exact {
152 return Ok(Resolved::One(id));
153 }
154
155 let mut stmt = conn.prepare(
156 "SELECT issue_id FROM leases WHERE repo = ?1 AND issue_id LIKE ?2 || '%' LIMIT 2",
157 )?;
158 let matches: Vec<String> = stmt
159 .query_map((repo, id_or_prefix), |row| row.get(0))?
160 .collect::<rusqlite::Result<_>>()?;
161 Ok(match matches.len() {
162 0 => Resolved::None,
163 1 => Resolved::One(matches.into_iter().next().unwrap_or_default()),
164 n => Resolved::Ambiguous(n),
165 })
166 }
167
112 /// Open (creating if needed) the lease database and ensure the schema. 168 /// Open (creating if needed) the lease database and ensure the schema.
113 pub fn open(path: &Path) -> rusqlite::Result<Connection> { 169 pub fn open(path: &Path) -> rusqlite::Result<Connection> {
114 let conn = Connection::open(path)?; 170 let conn = Connection::open(path)?;
@@ -249,6 +305,7 @@ pub fn release(
249 Some(row) if row.held(now) && row.holder.as_deref() != Some(holder) => Release::NotHolder { 305 Some(row) if row.held(now) && row.holder.as_deref() != Some(holder) => Release::NotHolder {
250 holder: row.holder.unwrap_or_default(), 306 holder: row.holder.unwrap_or_default(),
251 }, 307 },
308 Some(row) if row.holder.is_none() => Release::NotHeld,
252 Some(_) => { 309 Some(_) => {
253 // Free the row, keep the token. Deleting would reset the tenure 310 // Free the row, keep the token. Deleting would reset the tenure
254 // counter — see the module docs. 311 // counter — see the module docs.
@@ -259,9 +316,7 @@ pub fn release(
259 )?; 316 )?;
260 Release::Released 317 Release::Released
261 } 318 }
262 // Releasing nothing succeeds: a client retrying after a dropped 319 None => Release::NotHeld,
263 // connection must not fail.
264 None => Release::Released,
265 }; 320 };
266 tx.commit()?; 321 tx.commit()?;
267 Ok(outcome) 322 Ok(outcome)
@@ -439,11 +494,51 @@ mod tests {
439 } 494 }
440 495
441 #[test] 496 #[test]
442 fn release_idempotent_when_absent() { 497 fn release_of_nothing_succeeds_but_says_so() {
443 let mut c = mem(); 498 let mut c = mem();
499 // Still a success for a retrying client, but not called "released":
500 // reporting a freed lease when nothing was freed hid a real bug.
444 assert_eq!( 501 assert_eq!(
445 release(&mut c, "r", "i", "alice", 200).unwrap(), 502 release(&mut c, "r", "i", "alice", 200).unwrap(),
446 Release::Released 503 Release::NotHeld
504 );
505 }
506
507 #[test]
508 fn resolve_finds_an_exact_id_and_a_unique_prefix() {
509 let mut c = mem();
510 acquire(&mut c, "r", "abcdef123456", "alice", None, 100).unwrap();
511 assert_eq!(
512 resolve(&c, "r", "abcdef123456").unwrap(),
513 Resolved::One("abcdef123456".into())
514 );
515 assert_eq!(
516 resolve(&c, "r", "abcd").unwrap(),
517 Resolved::One("abcdef123456".into())
518 );
519 }
520
521 #[test]
522 fn resolve_reports_nothing_and_ambiguity() {
523 let mut c = mem();
524 acquire(&mut c, "r", "abcdef", "alice", None, 100).unwrap();
525 acquire(&mut c, "r", "abcxyz", "bob", None, 100).unwrap();
526 assert_eq!(resolve(&c, "r", "zz").unwrap(), Resolved::None);
527 assert_eq!(resolve(&c, "r", "abc").unwrap(), Resolved::Ambiguous(2));
528 // Another repo's rows are not candidates.
529 assert_eq!(resolve(&c, "other", "abc").unwrap(), Resolved::None);
530 }
531
532 #[test]
533 fn resolve_still_finds_a_freed_row_so_a_stale_id_is_answerable() {
534 let mut c = mem();
535 acquire(&mut c, "r", "abcdef", "alice", None, 100).unwrap();
536 release(&mut c, "r", "abcdef", "alice", 200).unwrap();
537 // The row survives to carry the token, so the id still resolves — and
538 // releasing it again answers NotHeld rather than inventing a lease.
539 assert_eq!(
540 resolve(&c, "r", "abc").unwrap(),
541 Resolved::One("abcdef".into())
447 ); 542 );
448 } 543 }
449 544
src/server/ssh/session.rs
Old New
@@ -441,10 +441,21 @@ impl SshHandler {
441 // UI uses, so a claim made here is the claim shown there. 441 // UI uses, so a claim made here is the claim shown there.
442 let repo_key = crate::leases::repo_key(&self.config.repos_dir, resolved_path); 442 let repo_key = crate::leases::repo_key(&self.config.repos_dir, resolved_path);
443 443
444 let mut conn = match crate::leases::open(&self.config.collab_db) {
445 Ok(c) => c,
446 Err(e) => {
447 error!("Failed to open the lease database: {}", e);
448 return reply_and_close(session, channel, "error: lease store unavailable\n", 1);
449 }
450 };
451
444 // Acquiring points at a specific issue, so the issue has to exist and 452 // Acquiring points at a specific issue, so the issue has to exist and
445 // be open: leasing work nobody can do is a bug we should refuse, not 453 // be open: leasing work nobody can do is a bug we should refuse, not
446 // record. Renew/release/list act on lease rows alone — an issue closed 454 // record. Renew/release act on lease rows alone — an issue closed
447 // mid-tenure must still be releasable. 455 // mid-tenure must still be releasable — so they resolve a prefix
456 // against the lease table instead of the issue refs. Both must resolve
457 // it *somehow*: taking the argument literally meant `release <prefix>`
458 // touched a key no row used and reported success (fixed 2026-09-06).
448 let issue_id = match &cmd { 459 let issue_id = match &cmd {
449 LeaseCmd::Acquire { issue, .. } => { 460 LeaseCmd::Acquire { issue, .. } => {
450 let repo = match git2::Repository::open(resolved_path) { 461 let repo = match git2::Repository::open(resolved_path) {
@@ -474,17 +485,30 @@ impl SshHandler {
474 } 485 }
475 } 486 }
476 } 487 }
477 LeaseCmd::Renew { issue, .. } | LeaseCmd::Release { issue, .. } => issue.clone(), 488 LeaseCmd::Renew { issue, .. } | LeaseCmd::Release { issue, .. } => {
489 match crate::leases::resolve(&conn, &repo_key, issue) {
490 Ok(crate::leases::Resolved::One(id)) => id,
491 // Nothing is leased under this id. Keep the argument so
492 // the operation still answers for it: release says
493 // "not-held", renew says "not-holder".
494 Ok(crate::leases::Resolved::None) => issue.clone(),
495 Ok(crate::leases::Resolved::Ambiguous(n)) => {
496 return reply_and_close(
497 session,
498 channel,
499 &format!("error: '{issue}' matches {n} leases; use a longer id\n"),
500 1,
501 );
502 }
503 Err(e) => {
504 error!("Failed to resolve {} in {}: {}", issue, repo_key, e);
505 return reply_and_close(session, channel, "error: lease store failed\n", 1);
506 }
507 }
508 }
478 LeaseCmd::List { .. } => String::new(), 509 LeaseCmd::List { .. } => String::new(),
479 }; 510 };
480 511
481 let mut conn = match crate::leases::open(&self.config.collab_db) {
482 Ok(c) => c,
483 Err(e) => {
484 error!("Failed to open the lease database: {}", e);
485 return reply_and_close(session, channel, "error: lease store unavailable\n", 1);
486 }
487 };
488 let now = unix_now() as i64; 512 let now = unix_now() as i64;
489 513
490 let (reply, code) = match lease_outcome(&mut conn, &cmd, &repo_key, &issue_id, &holder, now) 514 let (reply, code) = match lease_outcome(&mut conn, &cmd, &repo_key, &issue_id, &holder, now)
@@ -579,6 +603,15 @@ fn lease_outcome(
579 }), 603 }),
580 0, 604 0,
581 ), 605 ),
606 // Success, but honest: nothing was freed.
607 leases::Release::NotHeld => (
608 serde_json::json!({
609 "status": "not-held",
610 "repo": repo_key,
611 "issue": issue_id,
612 }),
613 0,
614 ),
582 leases::Release::NotHolder { holder } => ( 615 leases::Release::NotHolder { holder } => (
583 serde_json::json!({ 616 serde_json::json!({
584 "status": "not-holder", 617 "status": "not-holder",
tests/lease_server_test.rs
Old New
@@ -210,7 +210,7 @@ fn release_frees_the_issue_and_bumps_the_next_tenure() {
210 } 210 }
211 211
212 #[test] 212 #[test]
213 fn release_without_a_lease_succeeds() { 213 fn release_without_a_lease_succeeds_and_says_not_held() {
214 let (harness, id) = harness_with_issue("lease-release-noop"); 214 let (harness, id) = harness_with_issue("lease-release-noop");
215 215
216 // Idempotent: a client retrying after a dropped connection must not fail. 216 // Idempotent: a client retrying after a dropped connection must not fail.
@@ -219,7 +219,10 @@ fn release_without_a_lease_succeeds() {
219 id 219 id
220 )); 220 ));
221 assert_eq!(code(&out), 0); 221 assert_eq!(code(&out), 0);
222 assert_eq!(stdout_json(&out)["status"], "released"); 222 // This assertion used to read "released", which is what let the prefix bug
223 // of 2026-09-06 pass every test: a release that freed nothing reported the
224 // same status as one that freed a lease, so nothing could tell them apart.
225 assert_eq!(stdout_json(&out)["status"], "not-held");
223 } 226 }
224 227
225 #[test] 228 #[test]
@@ -317,3 +320,98 @@ fn read_only_principal_may_list_but_not_acquire() {
317 )); 320 ));
318 assert_eq!(code(&acquire), 1, "acquiring needs write"); 321 assert_eq!(code(&acquire), 1, "acquiring needs write");
319 } 322 }
323
324 /// A claim made by prefix must be releasable by that prefix.
325 ///
326 /// The regression test for the bug two real workers found on 2026-09-06:
327 /// lease rows are keyed by the full issue id, `acquire` resolved a prefix to
328 /// one, and `release` took its argument literally — so releasing by prefix
329 /// updated a key no row used and reported `released` anyway. The lease stayed
330 /// held until its TTL, blocking every other worker, and the CLI abbreviates
331 /// ids everywhere, so this was the ordinary path rather than a corner.
332 #[test]
333 fn a_claim_made_by_prefix_is_released_by_that_prefix() {
334 let (harness, id) = harness_with_issue("lease-prefix-release");
335 let first = harness.ssh_client_key();
336 let second = harness.second_authorized_key();
337 let prefix = &id[..8];
338
339 let claimed = harness.ssh_exec_as(
340 &first,
341 &format!("collab-lease acquire 'lease-prefix-release.git' '{prefix}' --ttl 300"),
342 b"",
343 );
344 assert_eq!(code(&claimed), 0);
345
346 let released = harness.ssh_exec_as(
347 &first,
348 &format!("collab-lease release 'lease-prefix-release.git' '{prefix}'"),
349 b"",
350 );
351 assert_eq!(code(&released), 0);
352 let json = stdout_json(&released);
353 assert_eq!(json["status"], "released", "a held lease must be freed");
354 // The reply names the full id, so a caller learns what it actually freed.
355 assert_eq!(json["issue"], id);
356
357 // The proof is not the reply: another worker must be able to take it.
358 let next = harness.ssh_exec_as(
359 &second,
360 &format!("collab-lease acquire 'lease-prefix-release.git' '{prefix}' --ttl 300"),
361 b"",
362 );
363 assert_eq!(
364 code(&next),
365 0,
366 "the issue must really be free, not merely reported free: {}",
367 String::from_utf8_lossy(&next.stdout)
368 );
369 assert_eq!(stdout_json(&next)["token"], 2, "a new tenure");
370 }
371
372 /// A prefix that matches two leases is refused rather than picking one.
373 #[test]
374 fn an_ambiguous_prefix_is_refused() {
375 let harness = ServerHarness::new("lease-ambiguous");
376 harness.push_head();
377 let repo = harness.work_repo_git2();
378
379 // Issue ids are content-derived, so collide them in the lease table
380 // directly by claiming two whole ids that share a prefix is not possible;
381 // instead claim two real issues and use a prefix short enough to match
382 // both only if they happen to share one. Find such a pair, or skip.
383 let mut ids = Vec::new();
384 for n in 0..12 {
385 let (_r, id) = common::open_issue(&repo, &common::alice(), &format!("Issue {n}"));
386 ids.push(id);
387 }
388 harness.push_collab_refs();
389
390 let shared = ids.iter().find_map(|a| {
391 ids.iter()
392 .find(|b| *b != a && b.as_bytes()[0] == a.as_bytes()[0])
393 .map(|b| (a.clone(), b.clone()))
394 });
395 let Some((a, b)) = shared else {
396 eprintln!("no two of 12 issue ids shared a first hex character; nothing to assert");
397 return;
398 };
399
400 for id in [&a, &b] {
401 let out = harness.ssh_exec(&format!(
402 "collab-lease acquire 'lease-ambiguous.git' '{id}' --ttl 300"
403 ));
404 assert_eq!(code(&out), 0);
405 }
406
407 let out = harness.ssh_exec(&format!(
408 "collab-lease release 'lease-ambiguous.git' '{}'",
409 &a[..1]
410 ));
411 assert_eq!(code(&out), 1, "an ambiguous prefix must not pick one");
412 assert!(
413 String::from_utf8_lossy(&out.stdout).contains("matches 2 leases"),
414 "stdout: {}",
415 String::from_utf8_lossy(&out.stdout)
416 );
417 }