Skip to content

Commit e2c87cd

Browse files
RoyLinRoyLin
authored andcommitted
fix(web): safely replace verified local server
1 parent 25675be commit e2c87cd

7 files changed

Lines changed: 343 additions & 42 deletions

File tree

README.md

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -529,10 +529,12 @@ Starting Web is idempotent. A healthy managed instance for the workspace is
529529
reused, and a healthy foreground or older A3S instance on the requested address
530530
is discovered without being killed. `status` and `open` can discover that
531531
unmanaged default-address instance, while `stop` refuses to signal it. Use
532-
`--replace` to gracefully restart only an authenticated managed instance. A
533-
foreign port owner is never terminated. Port binding and asset validation
534-
happen before configuration-heavy session restoration, so conflicts and broken
535-
installations fail quickly without unrelated restore warnings.
532+
`--replace` to restart an authenticated managed instance or a same-workspace
533+
foreground instance whose health PID, executable, `web` command, and explicit
534+
port all match the current invocation. A foreign or ambiguous port owner is
535+
never terminated. Port binding and asset validation happen before
536+
configuration-heavy session restoration, so conflicts and broken installations
537+
fail quickly without unrelated restore warnings.
536538

537539
Web tasks, visible messages, titles, goals, effort, model selection, and
538540
execution mode are saved under `~/.a3s/code-web` and restored before the API

docs/cli-product-design.md

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -343,9 +343,10 @@ managed instance associated with the canonical workspace. Stop and status
343343
validate process identity and never signal an unrelated process from a stale
344344
PID file. Repeated detached starts reuse a healthy same-workspace instance,
345345
concurrent starts converge under a workspace lock, and stale records are
346-
quarantined. `--replace` gracefully replaces only a CLI-managed instance; it
347-
does not kill an unknown port listener or take ownership of an A3S server
348-
started elsewhere.
346+
quarantined. `--replace` gracefully replaces a CLI-managed instance or a
347+
same-workspace foreground A3S Web process verified by health PID, executable,
348+
command, and explicit port. It does not kill an unknown or ambiguous port
349+
listener.
349350

350351
GitHub archives and Homebrew installations carry the matching Web workspace.
351352
When Cargo cannot install those data files, the first online Web start resolves
@@ -358,10 +359,12 @@ first-use download begins.
358359
Start is idempotent: it reuses a healthy workspace instance instead of treating
359360
repeat invocation as a failure. It may discover a healthy foreground or legacy
360361
A3S instance through the versioned health contract, but that observation does
361-
not grant lifecycle ownership. `--replace` performs authenticated graceful
362-
shutdown only for a managed instance; it refuses unmanaged A3S and foreign port
363-
owners. Port ownership is checked before assets, configuration, or persisted
364-
sessions are loaded.
362+
not grant general lifecycle ownership. `--replace` uses authenticated graceful
363+
shutdown for a managed instance. It may interrupt an observed foreground
364+
instance only after its workspace, health PID, executable, `web` command, and
365+
explicit requested port are verified twice around process inspection. Foreign
366+
and ambiguous port owners are refused. Port ownership is checked before assets,
367+
configuration, or persisted sessions are loaded.
365368

366369
Web sessions for the same canonical workspace share one Code Intelligence
367370
runtime. Monaco consumes typed status, outline, navigation, and diagnostics

docs/cli-technical-architecture.md

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -570,9 +570,13 @@ Before configuration or session restoration, foreground startup reserves the
570570
requested listener and detached startup probes any occupied address. A
571571
versioned A3S health response identifies healthy foreground and legacy
572572
instances for reuse and diagnostics, but it does not expose the control nonce
573-
or confer stop authority. `--replace` is accepted only for an authenticated
574-
managed record. A foreign listener and an unmanaged A3S listener are never
575-
signaled.
573+
or confer general stop authority. `--replace` uses the authenticated control
574+
route for a managed record. For an observed foreground instance, replacement
575+
additionally requires the same canonical workspace, a health-reported PID, the
576+
current A3S executable, a `web` command with the requested explicit port, and a
577+
second health probe immediately before signaling. The CLI sends an interrupt
578+
and waits for the listener to be released. A foreign or ambiguous listener is
579+
never signaled.
576580

577581
Foreground and detached modes use the same server configuration and startup
578582
path. Logs rotate under the shared state/log path policy and never contain

src/api/serve/background.rs

Lines changed: 260 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ use fs2::FileExt;
99
use rand::{rngs::OsRng, RngCore};
1010
use serde::{Deserialize, Serialize};
1111
use sha2::{Digest, Sha256};
12+
use sysinfo::{Pid, ProcessRefreshKind, ProcessesToUpdate, Signal, System, UpdateKind};
1213
use tokio::time::{sleep, Instant};
1314

1415
use super::options::ServeOptions;
@@ -18,6 +19,7 @@ pub(super) const INSTANCE_NONCE_ENV: &str = "A3S_INTERNAL_WEB_INSTANCE_NONCE";
1819
pub(super) const INSTANCE_FILE_ENV: &str = "A3S_INTERNAL_WEB_INSTANCE_FILE";
1920
const STARTUP_TIMEOUT: Duration = Duration::from_secs(15);
2021
const START_LOCK_TIMEOUT: Duration = Duration::from_secs(20);
22+
const OBSERVED_SHUTDOWN_TIMEOUT: Duration = Duration::from_secs(15);
2123
const POLL_INTERVAL: Duration = Duration::from_millis(50);
2224

2325
#[derive(Clone, Debug, Serialize, Deserialize)]
@@ -99,13 +101,10 @@ pub(super) async fn start(
99101

100102
if let Some(existing) = discover_requested_instance(options).await? {
101103
if options.replace {
102-
bail!(
103-
"A3S Web {} is healthy but is not managed by this CLI state; no process was \
104-
stopped. Stop its original command or managed service before using --replace",
105-
existing.address
106-
);
104+
replace_observed_instance(&existing, options, &executable).await?;
105+
} else {
106+
return Ok(BackgroundStart::Existing(existing));
107107
}
108-
return Ok(BackgroundStart::Existing(existing));
109108
}
110109
ensure_requested_port_available(options).await?;
111110
let _prepared_web_root = super::resolve_web_root(options).await?;
@@ -263,6 +262,233 @@ async fn stop_owned_instance(path: &Path, instance: &WebInstanceRecord) -> anyho
263262
bail!("A3S Web did not stop within 10 seconds; no force signal was sent")
264263
}
265264

265+
pub(super) async fn replace_observed_instance(
266+
existing: &WebEndpoint,
267+
options: &ServeOptions,
268+
executable: &Path,
269+
) -> anyhow::Result<()> {
270+
let pid = existing.pid.ok_or_else(|| {
271+
anyhow::anyhow!(
272+
"A3S Web {} did not report a process ID; no process was stopped",
273+
existing.address
274+
)
275+
})?;
276+
if pid == std::process::id() {
277+
bail!(
278+
"A3S Web {} reported the current process ID; no process was stopped",
279+
existing.address
280+
);
281+
}
282+
if options.addr.port() == 0 || existing.address.port() != options.addr.port() {
283+
bail!(
284+
"A3S Web {} does not match the requested port {}; no process was stopped",
285+
existing.address,
286+
options.addr.port()
287+
);
288+
}
289+
290+
let expected_executable = executable.to_path_buf();
291+
let expected_port = options.addr.port();
292+
let identity =
293+
inspect_observed_process(pid, expected_executable.clone(), expected_port).await?;
294+
295+
let confirmed = probe_web_endpoint(existing.address).await.ok_or_else(|| {
296+
anyhow::anyhow!(
297+
"A3S Web {} changed while replacement was being verified; no process was stopped",
298+
existing.address
299+
)
300+
})?;
301+
if confirmed.pid != Some(pid)
302+
|| !same_workspace(&existing.workspace, &confirmed.workspace)
303+
|| !same_workspace(&options.workspace, &confirmed.workspace)
304+
{
305+
bail!(
306+
"A3S Web {} changed while replacement was being verified; no process was stopped",
307+
existing.address
308+
);
309+
}
310+
311+
signal_observed_process(pid, expected_executable, expected_port, identity).await?;
312+
wait_until_port_released(existing.address).await
313+
}
314+
315+
#[derive(Clone, Copy)]
316+
struct ObservedProcessIdentity {
317+
start_time: u64,
318+
}
319+
320+
async fn inspect_observed_process(
321+
pid: u32,
322+
executable: PathBuf,
323+
port: u16,
324+
) -> anyhow::Result<ObservedProcessIdentity> {
325+
tokio::task::spawn_blocking(move || inspect_observed_process_sync(pid, &executable, port))
326+
.await
327+
.context("A3S Web process inspection task failed")?
328+
}
329+
330+
fn inspect_observed_process_sync(
331+
pid: u32,
332+
executable: &Path,
333+
port: u16,
334+
) -> anyhow::Result<ObservedProcessIdentity> {
335+
let system = process_snapshot(pid);
336+
let process = system.process(Pid::from_u32(pid)).ok_or_else(|| {
337+
anyhow::anyhow!("A3S Web process {pid} is no longer running; no process was stopped")
338+
})?;
339+
verify_observed_process(process, executable, port)?;
340+
Ok(ObservedProcessIdentity {
341+
start_time: process.start_time(),
342+
})
343+
}
344+
345+
async fn signal_observed_process(
346+
pid: u32,
347+
executable: PathBuf,
348+
port: u16,
349+
identity: ObservedProcessIdentity,
350+
) -> anyhow::Result<()> {
351+
tokio::task::spawn_blocking(move || {
352+
let system = process_snapshot(pid);
353+
let Some(process) = system.process(Pid::from_u32(pid)) else {
354+
return Ok(());
355+
};
356+
verify_observed_process(process, &executable, port)?;
357+
if process.start_time() != identity.start_time {
358+
bail!("A3S Web process {pid} changed identity; no process was stopped");
359+
}
360+
match process.kill_with(Signal::Interrupt) {
361+
Some(true) => Ok(()),
362+
Some(false) => bail!(
363+
"could not interrupt verified A3S Web process {pid}; no force signal was sent"
364+
),
365+
None if process.kill() => Ok(()),
366+
None => bail!("could not stop verified A3S Web process {pid}"),
367+
}
368+
})
369+
.await
370+
.context("A3S Web process stop task failed")?
371+
}
372+
373+
fn process_snapshot(pid: u32) -> System {
374+
let pid = Pid::from_u32(pid);
375+
let pids = [pid];
376+
let mut system = System::new();
377+
system.refresh_processes_specifics(
378+
ProcessesToUpdate::Some(&pids),
379+
true,
380+
ProcessRefreshKind::nothing()
381+
.with_cmd(UpdateKind::Always)
382+
.with_exe(UpdateKind::Always)
383+
.without_tasks(),
384+
);
385+
system
386+
}
387+
388+
fn verify_observed_process(
389+
process: &sysinfo::Process,
390+
executable: &Path,
391+
port: u16,
392+
) -> anyhow::Result<()> {
393+
let pid = process.pid().as_u32();
394+
let Some(process_executable) = process.exe() else {
395+
bail!("could not verify the executable for A3S Web process {pid}; no process was stopped");
396+
};
397+
if !same_executable(process_executable, executable) {
398+
bail!(
399+
"A3S Web process {pid} does not use the current a3s executable; no process was stopped"
400+
);
401+
}
402+
if !command_declares_web_port(process.cmd(), port) {
403+
bail!("A3S Web process {pid} does not declare `web --port {port}`; no process was stopped");
404+
}
405+
Ok(())
406+
}
407+
408+
fn same_executable(left: &Path, right: &Path) -> bool {
409+
let left = comparable_executable_path(left);
410+
let right = comparable_executable_path(right);
411+
match (left.canonicalize(), right.canonicalize()) {
412+
(Ok(left), Ok(right)) => left == right,
413+
_ => left == right,
414+
}
415+
}
416+
417+
#[cfg(target_os = "linux")]
418+
fn comparable_executable_path(path: &Path) -> PathBuf {
419+
use std::ffi::OsString;
420+
use std::os::unix::ffi::{OsStrExt, OsStringExt};
421+
422+
const DELETED_SUFFIX: &[u8] = b" (deleted)";
423+
let bytes = path.as_os_str().as_bytes();
424+
let bytes = bytes.strip_suffix(DELETED_SUFFIX).unwrap_or(bytes);
425+
PathBuf::from(OsString::from_vec(bytes.to_vec()))
426+
}
427+
428+
#[cfg(not(target_os = "linux"))]
429+
fn comparable_executable_path(path: &Path) -> PathBuf {
430+
path.to_path_buf()
431+
}
432+
433+
fn command_declares_web_port(command: &[std::ffi::OsString], expected_port: u16) -> bool {
434+
let Some(web_index) = command
435+
.iter()
436+
.position(|argument| argument.to_string_lossy() == "web")
437+
else {
438+
return false;
439+
};
440+
let mut declared_port = None;
441+
let mut index = web_index + 1;
442+
while index < command.len() {
443+
let argument = command[index].to_string_lossy();
444+
if argument == "--port" {
445+
let Some(value) = command.get(index + 1).and_then(|value| value.to_str()) else {
446+
return false;
447+
};
448+
let Ok(port) = value.parse::<u16>() else {
449+
return false;
450+
};
451+
declared_port = Some(port);
452+
index += 2;
453+
continue;
454+
}
455+
if let Some(value) = argument.strip_prefix("--port=") {
456+
let Ok(port) = value.parse::<u16>() else {
457+
return false;
458+
};
459+
declared_port = Some(port);
460+
}
461+
index += 1;
462+
}
463+
declared_port == Some(expected_port)
464+
}
465+
466+
async fn wait_until_port_released(address: SocketAddr) -> anyhow::Result<()> {
467+
let deadline = Instant::now() + OBSERVED_SHUTDOWN_TIMEOUT;
468+
loop {
469+
match tokio::net::TcpListener::bind(address).await {
470+
Ok(listener) => {
471+
drop(listener);
472+
return Ok(());
473+
}
474+
Err(error) if error.kind() == std::io::ErrorKind::AddrInUse => {
475+
if Instant::now() >= deadline {
476+
bail!(
477+
"verified A3S Web process did not release {} within {} seconds",
478+
address,
479+
OBSERVED_SHUTDOWN_TIMEOUT.as_secs()
480+
);
481+
}
482+
}
483+
Err(error) => {
484+
return Err(error)
485+
.with_context(|| format!("failed to verify that A3S Web released {address}"))
486+
}
487+
}
488+
sleep(POLL_INTERVAL).await;
489+
}
490+
}
491+
266492
pub(crate) async fn open(workspace: &Path) -> anyhow::Result<WebEndpoint> {
267493
let status = status(workspace).await?;
268494
if status.running {
@@ -697,6 +923,7 @@ fn configure_detached(_command: &mut Command) {}
697923
#[cfg(test)]
698924
mod tests {
699925
use super::*;
926+
use std::ffi::OsString;
700927

701928
#[test]
702929
fn foreground_arguments_remove_parent_only_lifecycle_flags() {
@@ -714,4 +941,31 @@ mod tests {
714941
["--host", "127.0.0.1", "--port", "29653"]
715942
);
716943
}
944+
945+
#[test]
946+
fn observed_replacement_requires_web_with_the_matching_explicit_port() {
947+
let matching = ["a3s", "web", "--host", "127.0.0.1", "--port", "29653"]
948+
.into_iter()
949+
.map(OsString::from)
950+
.collect::<Vec<_>>();
951+
assert!(command_declares_web_port(&matching, 29653));
952+
953+
let environment_only = ["a3s", "web"]
954+
.into_iter()
955+
.map(OsString::from)
956+
.collect::<Vec<_>>();
957+
assert!(!command_declares_web_port(&environment_only, 29653));
958+
959+
let wrong_port = ["a3s", "web", "--port", "29654"]
960+
.into_iter()
961+
.map(OsString::from)
962+
.collect::<Vec<_>>();
963+
assert!(!command_declares_web_port(&wrong_port, 29653));
964+
965+
let wrong_command = ["a3s", "code", "--port", "29653"]
966+
.into_iter()
967+
.map(OsString::from)
968+
.collect::<Vec<_>>();
969+
assert!(!command_declares_web_port(&wrong_command, 29653));
970+
}
717971
}

0 commit comments

Comments
 (0)