Compare commits
7
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
d647b56b43 | ||
|
|
a3840f7263 | ||
|
|
f662ed04ce | ||
|
|
84a5757c74 | ||
|
|
73a6e3d8b4 | ||
|
|
943c83b9e3 | ||
|
|
60188610ee |
@@ -32,11 +32,28 @@ pub async fn set_browser_view_enabled(
|
|||||||
// Persist first, then tear down: the supervisor's own teardown emit
|
// Persist first, then tear down: the supervisor's own teardown emit
|
||||||
// reads this flag back out of the store, and reading it mid-stop would
|
// reads this flag back out of the store, and reading it mid-stop would
|
||||||
// announce a view that is going away as still enabled.
|
// announce a view that is going away as still enabled.
|
||||||
state
|
//
|
||||||
|
// But the write's outcome is a *value*, not a branch. A `?` here meant
|
||||||
|
// that a store with no such project record returned early and
|
||||||
|
// `manager().stop()` never ran, leaving the supervisor, the proxy and
|
||||||
|
// the host port up for a project that, as far as the user is concerned,
|
||||||
|
// just had its view switched off. That state is not hypothetical while
|
||||||
|
// a session is live — the supervisor's own `store.get()` check in
|
||||||
|
// [`crate::browser_view`] exists because a record can go away
|
||||||
|
// underneath it — and before the flag was persisted at all, turning the
|
||||||
|
// view off always tore the session down.
|
||||||
|
let persisted = state
|
||||||
.projects_store
|
.projects_store
|
||||||
.set_browser_view_enabled(&project_id, false)?;
|
.set_browser_view_enabled(&project_id, false);
|
||||||
// Awaits the supervisor, so the host port is released before we return.
|
// Awaits the supervisor, so the host port is released before we return.
|
||||||
manager().stop(&project_id).await;
|
//
|
||||||
|
// A failed write is still reported rather than logged and swallowed.
|
||||||
|
// The resources are gone either way by this point, so surfacing it
|
||||||
|
// costs nothing that matters, and the failure it describes is one the
|
||||||
|
// user needs: the stored flag still says *enabled*, so the view comes
|
||||||
|
// back by itself on the next launch. Returning `Ok` would be a claim
|
||||||
|
// about persistence that isn't true.
|
||||||
|
tear_down_then_report(persisted, manager().stop(&project_id)).await?;
|
||||||
return Ok(manager().status(&project_id, false).await);
|
return Ok(manager().status(&project_id, false).await);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -52,6 +69,20 @@ pub async fn set_browser_view_enabled(
|
|||||||
.await
|
.await
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Await `teardown`, then report `persisted`.
|
||||||
|
///
|
||||||
|
/// Trivial on purpose, and split out for one reason: it is the whole rule the
|
||||||
|
/// disable path of [`set_browser_view_enabled`] has to obey — the teardown is
|
||||||
|
/// unconditional, and a failed persist surfaces only after it has run — and as
|
||||||
|
/// a free function that rule can be tested without a live `AppState`.
|
||||||
|
async fn tear_down_then_report(
|
||||||
|
persisted: Result<(), String>,
|
||||||
|
teardown: impl std::future::Future<Output = ()>,
|
||||||
|
) -> Result<(), String> {
|
||||||
|
teardown.await;
|
||||||
|
persisted
|
||||||
|
}
|
||||||
|
|
||||||
/// Current status. Cheap: the session map in this process plus the stored flag,
|
/// Current status. Cheap: the session map in this process plus the stored flag,
|
||||||
/// never the container.
|
/// never the container.
|
||||||
///
|
///
|
||||||
@@ -383,3 +414,43 @@ async fn running_container(
|
|||||||
}
|
}
|
||||||
Ok(container_id)
|
Ok(container_id)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use super::*;
|
||||||
|
use std::sync::atomic::{AtomicBool, Ordering};
|
||||||
|
|
||||||
|
/// The regression: turning the view off must not leave the supervisor, the
|
||||||
|
/// proxy and the host port running just because the project record could
|
||||||
|
/// not be written — which is exactly what a missing record did.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn a_failed_persist_does_not_skip_the_teardown() {
|
||||||
|
let torn_down = AtomicBool::new(false);
|
||||||
|
let result = tear_down_then_report(Err("Project x not found".to_string()), async {
|
||||||
|
torn_down.store(true, Ordering::SeqCst);
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
torn_down.load(Ordering::SeqCst),
|
||||||
|
"the session must be torn down even when the store write failed"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
result.err().as_deref(),
|
||||||
|
Some("Project x not found"),
|
||||||
|
"and the write failure must still reach the caller, not be swallowed"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn a_successful_persist_reports_success_after_the_teardown() {
|
||||||
|
let torn_down = AtomicBool::new(false);
|
||||||
|
let result = tear_down_then_report(Ok(()), async {
|
||||||
|
torn_down.store(true, Ordering::SeqCst);
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
|
||||||
|
assert!(torn_down.load(Ordering::SeqCst));
|
||||||
|
assert!(result.is_ok());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -101,25 +101,56 @@ fn pick_recorded_lineage(
|
|||||||
/// snapshot to fall back on.
|
/// snapshot to fall back on.
|
||||||
const NOTHING_TO_PROBE: &str = "This project has no container or snapshot image yet, so there is nothing to compare against the base image.";
|
const NOTHING_TO_PROBE: &str = "This project has no container or snapshot image yet, so there is nothing to compare against the base image.";
|
||||||
|
|
||||||
|
/// The project's container, as the daemon reported it.
|
||||||
|
///
|
||||||
|
/// `running` lives *inside* `Present` because it is only ever read about a
|
||||||
|
/// container that was found: `is_container_running` needs an id. Keeping the
|
||||||
|
/// two in one variant makes "running, but no container" unrepresentable rather
|
||||||
|
/// than merely unreached, which is what [`pick_probe_source`] relies on when it
|
||||||
|
/// hands a container id to the container probe arms.
|
||||||
|
#[derive(Debug, PartialEq, Eq)]
|
||||||
|
enum ContainerState {
|
||||||
|
/// The project genuinely has no container — an answer, not a failure to
|
||||||
|
/// look.
|
||||||
|
Absent,
|
||||||
|
Present {
|
||||||
|
id: String,
|
||||||
|
running: bool,
|
||||||
|
},
|
||||||
|
}
|
||||||
|
|
||||||
|
impl ContainerState {
|
||||||
|
fn id(&self) -> Option<&str> {
|
||||||
|
match self {
|
||||||
|
ContainerState::Absent => None,
|
||||||
|
ContainerState::Present { id, .. } => Some(id),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Where [`get_container_staleness`] reads the project's *current* filesystem
|
/// Where [`get_container_staleness`] reads the project's *current* filesystem
|
||||||
/// from, in descending order of how current the answer is.
|
/// from, in descending order of how current the answer is.
|
||||||
|
///
|
||||||
|
/// The container variants carry the id they will be probed with, so that
|
||||||
|
/// "there is a container to read" and "here is which one" cannot come apart
|
||||||
|
/// downstream.
|
||||||
#[derive(Debug, PartialEq, Eq)]
|
#[derive(Debug, PartialEq, Eq)]
|
||||||
enum ProbeSource {
|
enum ProbeSource<'a> {
|
||||||
/// `docker exec` into the live container. The only source that includes
|
/// `docker exec` into the live container. The only source that includes
|
||||||
/// everything installed since the last commit *in this session*.
|
/// everything installed since the last commit *in this session*.
|
||||||
RunningContainer,
|
RunningContainer(&'a str),
|
||||||
/// Commit the stopped container's writable layer to a throwaway image and
|
/// Commit the stopped container's writable layer to a throwaway image and
|
||||||
/// probe that. Exactly as current as the container, which is what makes it
|
/// probe that. Exactly as current as the container, which is what makes it
|
||||||
/// preferable to the snapshot — see below.
|
/// preferable to the snapshot — see below.
|
||||||
StoppedContainer,
|
StoppedContainer(&'a str),
|
||||||
/// A throwaway container from `triple-c-snapshot-<id>:latest`.
|
/// A throwaway container from `triple-c-snapshot-<id>:latest`.
|
||||||
Snapshot,
|
Snapshot,
|
||||||
/// Nothing to read: no container, no snapshot.
|
/// Nothing to read: no container, no snapshot.
|
||||||
Nothing,
|
Nothing,
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Pick the probe source. `container_running` is `None` when the project has no
|
/// Pick the probe source, or report the one reading this decision needed and
|
||||||
/// container at all, `Some(false)` when it has a stopped one.
|
/// did not get.
|
||||||
///
|
///
|
||||||
/// **A stopped container outranks the snapshot.** The snapshot image is not a
|
/// **A stopped container outranks the snapshot.** The snapshot image is not a
|
||||||
/// checkpoint — `commit_container_snapshot` runs only before a removal (a
|
/// checkpoint — `commit_container_snapshot` runs only before a removal (a
|
||||||
@@ -133,12 +164,30 @@ enum ProbeSource {
|
|||||||
/// Getting this wrong is what made a stopped, never-recreated project report
|
/// Getting this wrong is what made a stopped, never-recreated project report
|
||||||
/// "no container or snapshot image yet" — with its container sitting right
|
/// "no container or snapshot image yet" — with its container sitting right
|
||||||
/// there — and left Update disabled on the projects that most needed it.
|
/// there — and left Update disabled on the projects that most needed it.
|
||||||
fn pick_probe_source(container_running: Option<bool>, snapshot_exists: bool) -> ProbeSource {
|
///
|
||||||
match (container_running, snapshot_exists) {
|
/// **`snapshot_exists` is consulted only where it decides something.** When a
|
||||||
(Some(true), _) => ProbeSource::RunningContainer,
|
/// container answered, the snapshot is not part of this decision at all, so a
|
||||||
(Some(false), _) => ProbeSource::StoppedContainer,
|
/// failed `image_exists` is passed over rather than surfaced: destroying a
|
||||||
(None, true) => ProbeSource::Snapshot,
|
/// report the running container could have supplied in full would be the same
|
||||||
(None, false) => ProbeSource::Nothing,
|
/// mistake, in the other direction, as reading an unreachable daemon as an
|
||||||
|
/// absent container. It is load-bearing only with no container at all, and
|
||||||
|
/// there its failure *is* the answer this function cannot give.
|
||||||
|
fn pick_probe_source<'a>(
|
||||||
|
container: &'a ContainerState,
|
||||||
|
snapshot_exists: &Result<bool, String>,
|
||||||
|
) -> Result<ProbeSource<'a>, String> {
|
||||||
|
match container {
|
||||||
|
ContainerState::Present { id, running: true } => Ok(ProbeSource::RunningContainer(id)),
|
||||||
|
// The stopped path may still want the snapshot, but only as a fallback
|
||||||
|
// it can do without — see `stopped_probe_policy` and the commit-failure
|
||||||
|
// arm in `get_container_staleness`, which each handle an unreadable
|
||||||
|
// snapshot themselves.
|
||||||
|
ContainerState::Present { id, running: false } => Ok(ProbeSource::StoppedContainer(id)),
|
||||||
|
ContainerState::Absent => match snapshot_exists {
|
||||||
|
Ok(true) => Ok(ProbeSource::Snapshot),
|
||||||
|
Ok(false) => Ok(ProbeSource::Nothing),
|
||||||
|
Err(e) => Err(probe_failed(e)),
|
||||||
|
},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -159,8 +208,8 @@ enum StoppedProbe {
|
|||||||
/// touches nothing, which is what makes it the right answer while another
|
/// touches nothing, which is what makes it the right answer while another
|
||||||
/// operation owns the container.
|
/// operation owns the container.
|
||||||
SnapshotInstead,
|
SnapshotInstead,
|
||||||
/// Report rather than guess.
|
/// Report rather than guess, with the message to report.
|
||||||
Defer,
|
Defer(String),
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Pick what to do about a stopped container.
|
/// Pick what to do about a stopped container.
|
||||||
@@ -173,14 +222,190 @@ enum StoppedProbe {
|
|||||||
/// container with a hard `?`, so a non-404 from a remove that raced this commit
|
/// container with a hard `?`, so a non-404 from a remove that raced this commit
|
||||||
/// fails the whole Start with an opaque "Failed to remove container". Reading
|
/// fails the whole Start with an opaque "Failed to remove container". Reading
|
||||||
/// the claim costs nothing and takes that failure off the table.
|
/// the claim costs nothing and takes that failure off the table.
|
||||||
fn stopped_probe_policy(project_is_busy: bool, snapshot_exists: bool) -> StoppedProbe {
|
///
|
||||||
|
/// `snapshot_exists` matters only once the project is busy, because that is the
|
||||||
|
/// only state in which the snapshot is the alternative to committing. An
|
||||||
|
/// unreadable snapshot there leaves nothing to fall back *to*, so its error is
|
||||||
|
/// what gets reported: "try again once it finishes" alone would be a claim that
|
||||||
|
/// waiting is all that stands in the way, which a failed `image_exists` has not
|
||||||
|
/// established.
|
||||||
|
fn stopped_probe_policy(
|
||||||
|
project_is_busy: bool,
|
||||||
|
snapshot_exists: &Result<bool, String>,
|
||||||
|
) -> StoppedProbe {
|
||||||
match (project_is_busy, snapshot_exists) {
|
match (project_is_busy, snapshot_exists) {
|
||||||
(false, _) => StoppedProbe::Commit,
|
(false, _) => StoppedProbe::Commit,
|
||||||
(true, true) => StoppedProbe::SnapshotInstead,
|
(true, Ok(true)) => StoppedProbe::SnapshotInstead,
|
||||||
(true, false) => StoppedProbe::Defer,
|
(true, Ok(false)) => StoppedProbe::Defer(PROJECT_BUSY.to_string()),
|
||||||
|
(true, Err(e)) => StoppedProbe::Defer(probe_failed(e)),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Reported as `probe_error` when a probe input could not be read at all.
|
||||||
|
///
|
||||||
|
/// Deliberately distinct from [`NOTHING_TO_PROBE`]: a failed reading is not
|
||||||
|
/// evidence that the project has no container, and saying "no container or
|
||||||
|
/// snapshot image yet" on a transient fault was confidently wrong about a
|
||||||
|
/// project that may well have both.
|
||||||
|
///
|
||||||
|
/// Deliberately *neutral about the cause*, too. Only one of the four readings
|
||||||
|
/// implies an unreachable daemon: `mig::image_id` maps a 404 to `Ok(None)` and
|
||||||
|
/// returns `Err` for any other status, and `find_existing_container` /
|
||||||
|
/// `image_exists` wrap every list failure the same way — all of which a daemon
|
||||||
|
/// that answered perfectly well can produce. The base image name comes from
|
||||||
|
/// user settings, so a malformed reference alone reaches here, and telling that
|
||||||
|
/// user to go fix a running daemon would be the same unestablished claim about
|
||||||
|
/// a cause that this whole probe path exists to stop making.
|
||||||
|
///
|
||||||
|
/// The underlying error is carried through verbatim, because "Docker is not
|
||||||
|
/// running" and "permission denied on /var/run/docker.sock" call for different
|
||||||
|
/// fixes from the user.
|
||||||
|
///
|
||||||
|
/// The sentence names *the check*, not the container, because only two of the
|
||||||
|
/// four readings are about the container at all — the other two are the base
|
||||||
|
/// image and the snapshot image. Saying "this project's container could not be
|
||||||
|
/// inspected" for a malformed base image name in settings would point the user
|
||||||
|
/// at the wrong object, which is the same mistake one size down.
|
||||||
|
fn probe_failed(e: &str) -> String {
|
||||||
|
format!("This project could not be checked against its base image: {}", e)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The four daemon readings [`get_container_staleness`] takes before it can
|
||||||
|
/// choose a probe source, each still carrying whether it is an answer.
|
||||||
|
///
|
||||||
|
/// **Absence and unreachability are different answers, and only one of them is
|
||||||
|
/// an answer.** All four callees already draw that line — `image_id` maps a 404
|
||||||
|
/// to `Ok(None)`, `find_existing_container` and `image_exists` return `Ok` with
|
||||||
|
/// an empty filtered list — so a call site that writes `.unwrap_or(None)` /
|
||||||
|
/// `.unwrap_or(false)` is not defaulting, it is *discarding a distinction the
|
||||||
|
/// callee went to the trouble of making*. That is what let an unreachable
|
||||||
|
/// daemon reach [`pick_probe_source`] as "no container, no snapshot" and report
|
||||||
|
/// [`NOTHING_TO_PROBE`] — a confident claim about a project nothing had
|
||||||
|
/// actually looked at.
|
||||||
|
///
|
||||||
|
/// The `Result` fields are the guard against that returning: the call site
|
||||||
|
/// hands over what the daemon said, unmodified, and an `.unwrap_or` there no
|
||||||
|
/// longer type-checks.
|
||||||
|
#[derive(Debug)]
|
||||||
|
struct ProbeReadings {
|
||||||
|
/// `docker::find_existing_container`.
|
||||||
|
container_id: Result<Option<String>, String>,
|
||||||
|
/// `docker::is_container_running`, and `None` when there was no container
|
||||||
|
/// to ask about — not a swallowed error.
|
||||||
|
container_running: Option<Result<bool, String>>,
|
||||||
|
/// `mig::image_id` for the configured base image.
|
||||||
|
base_image_id: Result<Option<String>, String>,
|
||||||
|
/// `docker::image_exists` for the project's snapshot image.
|
||||||
|
snapshot_exists: Result<bool, String>,
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The readings [`get_container_staleness`] carries past the point where a
|
||||||
|
/// missing one would have stopped it.
|
||||||
|
#[derive(Debug)]
|
||||||
|
struct ProbeInputs {
|
||||||
|
/// The current base image's ID, or `None` when it is not pulled locally —
|
||||||
|
/// which [`mig::image_id`] reports as `Ok(None)`, not an error.
|
||||||
|
current_base_image_id: Option<String>,
|
||||||
|
container: ContainerState,
|
||||||
|
/// Still a `Result`, because whether it is load-bearing depends on the
|
||||||
|
/// container: see [`pick_probe_source`].
|
||||||
|
snapshot_exists: Result<bool, String>,
|
||||||
|
}
|
||||||
|
|
||||||
|
/// What [`get_container_staleness`] does next, once the readings are in.
|
||||||
|
#[derive(Debug)]
|
||||||
|
enum ProbeStart {
|
||||||
|
/// Go ahead, with these inputs.
|
||||||
|
Inputs(Box<ProbeInputs>),
|
||||||
|
/// Stop, and hand the user this report.
|
||||||
|
///
|
||||||
|
/// **Reported, not returned.** The hook's `catch` sets `staleness` to
|
||||||
|
/// `null`, and `ContainerMigrationBanner` renders nothing at all for a null
|
||||||
|
/// staleness — so an `Err` out of the command would make the banner vanish
|
||||||
|
/// at exactly the moment it has something to say. A `probe_error` on an
|
||||||
|
/// otherwise-default report keeps it on screen, reading "Container base
|
||||||
|
/// could not be checked". Carrying a `ContainerStaleness` rather than an
|
||||||
|
/// error string is what keeps that decision here, where it is tested,
|
||||||
|
/// instead of in the `?` someone adds at the call site later.
|
||||||
|
Report(Box<ContainerStaleness>),
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Decide whether the collected readings are enough to probe with.
|
||||||
|
///
|
||||||
|
/// Only the readings this decision actually rests on can stop it:
|
||||||
|
///
|
||||||
|
/// * `container_id` selects the probe source outright, so a failure to read it
|
||||||
|
/// leaves nothing to choose between. Fatal.
|
||||||
|
/// * `base_image_id` is fatal too, and deliberately so: it is the right-hand
|
||||||
|
/// side of the staleness comparison, where `None` ("not pulled locally", an
|
||||||
|
/// answer) and `Err` ("could not ask") both otherwise collapse into
|
||||||
|
/// `stale: false`. Reporting a project as up to date because the base image
|
||||||
|
/// could not be read is exactly the #56 mistake, one field over.
|
||||||
|
/// * `container_running` is asked only about a container that was found, and
|
||||||
|
/// decides between two live probe sources. Fatal when present.
|
||||||
|
/// * `snapshot_exists` is *not* fatal here, because it is load-bearing in only
|
||||||
|
/// two of the downstream states — no container at all, and a stopped
|
||||||
|
/// container on a busy project. It travels as a `Result` so each of those can
|
||||||
|
/// surface it, and the states that never consult it are not punished for it.
|
||||||
|
///
|
||||||
|
/// The first error wins, because when the daemon is unreachable they fail
|
||||||
|
/// together and the user needs the reason once, not three times. The order is
|
||||||
|
/// `container_id`, then `base_image_id`, then `container_running` — chosen
|
||||||
|
/// priority, deliberately *not* the order the daemon was called in, so that the
|
||||||
|
/// reported error is most often the one that stopped the probe rather than
|
||||||
|
/// whichever reading happened to run first. (It is at most three, not four:
|
||||||
|
/// `container_running` is only attempted when `container_id` answered with a
|
||||||
|
/// container.)
|
||||||
|
///
|
||||||
|
/// **A caveat this cannot fix here.** `docker::is_container_running` swallows
|
||||||
|
/// `inspect_container` failures into `Ok(false)` itself and errors only when the
|
||||||
|
/// client cannot be built, so a daemon that dies between the list and the
|
||||||
|
/// inspect still reads as "stopped" rather than as an error. That is a fix
|
||||||
|
/// inside that function, not at this call site; threading its `Result` through
|
||||||
|
/// at least stops *this* layer from adding a second swallow on top.
|
||||||
|
fn start_probe(readings: ProbeReadings) -> ProbeStart {
|
||||||
|
match collect_probe_inputs(readings) {
|
||||||
|
Ok(inputs) => ProbeStart::Inputs(Box::new(inputs)),
|
||||||
|
Err(e) => ProbeStart::Report(Box::new(ContainerStaleness {
|
||||||
|
probe_error: Some(e),
|
||||||
|
..Default::default()
|
||||||
|
})),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn collect_probe_inputs(readings: ProbeReadings) -> Result<ProbeInputs, String> {
|
||||||
|
let ProbeReadings {
|
||||||
|
container_id,
|
||||||
|
container_running,
|
||||||
|
base_image_id,
|
||||||
|
snapshot_exists,
|
||||||
|
} = readings;
|
||||||
|
|
||||||
|
let container_id = container_id.map_err(|e| probe_failed(&e))?;
|
||||||
|
let current_base_image_id = base_image_id.map_err(|e| probe_failed(&e))?;
|
||||||
|
let container_running = container_running
|
||||||
|
.transpose()
|
||||||
|
.map_err(|e| probe_failed(&e))?;
|
||||||
|
|
||||||
|
let container = match (container_id, container_running) {
|
||||||
|
(Some(id), Some(running)) => ContainerState::Present { id, running },
|
||||||
|
// No container: whatever `container_running` says is about nothing, and
|
||||||
|
// the caller only produces `None` here anyway.
|
||||||
|
(None, _) => ContainerState::Absent,
|
||||||
|
// A container was found but nobody asked whether it was running. The
|
||||||
|
// caller cannot produce this, and guessing "stopped" would cost a
|
||||||
|
// running project the only probe source that sees this session's
|
||||||
|
// installs — so say what happened instead.
|
||||||
|
(Some(_), None) => return Err(probe_failed("the container's state was not read")),
|
||||||
|
};
|
||||||
|
|
||||||
|
Ok(ProbeInputs {
|
||||||
|
current_base_image_id,
|
||||||
|
container,
|
||||||
|
snapshot_exists,
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
/// Runs two filesystem probes (~3 s each) and is therefore meant to be called
|
/// Runs two filesystem probes (~3 s each) and is therefore meant to be called
|
||||||
/// on demand, not polled.
|
/// on demand, not polled.
|
||||||
///
|
///
|
||||||
@@ -214,7 +439,35 @@ pub async fn get_container_staleness(
|
|||||||
let snapshot_image = docker::get_snapshot_image_name(&project);
|
let snapshot_image = docker::get_snapshot_image_name(&project);
|
||||||
|
|
||||||
let mut out = ContainerStaleness::default();
|
let mut out = ContainerStaleness::default();
|
||||||
out.current_base_image_id = mig::image_id(&base_image).await.unwrap_or(None);
|
|
||||||
|
// Every reading the daemon owes us, taken up front and handed on exactly as
|
||||||
|
// it came back, so that "could not ask" stays distinguishable from "asked,
|
||||||
|
// and the answer is no". [`start_probe`] is where that distinction is acted
|
||||||
|
// on; nothing between here and there may collapse one into the other, and
|
||||||
|
// the `Result` fields of [`ProbeReadings`] are what stop it being possible.
|
||||||
|
let container_id_result = docker::find_existing_container(&project).await;
|
||||||
|
let container_running_result = match &container_id_result {
|
||||||
|
Ok(Some(id)) => Some(docker::is_container_running(id).await),
|
||||||
|
// No container, or no usable reading of one: nothing to inspect, and
|
||||||
|
// the container lookup's own error is what gets reported.
|
||||||
|
_ => None,
|
||||||
|
};
|
||||||
|
let readings = ProbeReadings {
|
||||||
|
container_id: container_id_result,
|
||||||
|
container_running: container_running_result,
|
||||||
|
base_image_id: mig::image_id(&base_image).await,
|
||||||
|
snapshot_exists: docker::image_exists(&snapshot_image).await,
|
||||||
|
};
|
||||||
|
|
||||||
|
let inputs = match start_probe(readings) {
|
||||||
|
ProbeStart::Inputs(inputs) => *inputs,
|
||||||
|
// Reported, not returned — see [`ProbeStart::Report`].
|
||||||
|
ProbeStart::Report(report) => return Ok(*report),
|
||||||
|
};
|
||||||
|
let container = inputs.container;
|
||||||
|
let container_id = container.id();
|
||||||
|
|
||||||
|
out.current_base_image_id = inputs.current_base_image_id;
|
||||||
out.snapshot_created_at = mig::image_created(&snapshot_image).await;
|
out.snapshot_created_at = mig::image_created(&snapshot_image).await;
|
||||||
|
|
||||||
// Lineage, most authoritative source first: the live container's label,
|
// Lineage, most authoritative source first: the live container's label,
|
||||||
@@ -228,8 +481,7 @@ pub async fn get_container_staleness(
|
|||||||
// as an answer and skip the snapshot entirely, so a snapshot that *did*
|
// as an answer and skip the snapshot entirely, so a snapshot that *did*
|
||||||
// record a lineage was never consulted and the project reported "unknown"
|
// record a lineage was never consulted and the project reported "unknown"
|
||||||
// with the information sitting one lookup away.
|
// with the information sitting one lookup away.
|
||||||
let container_id = docker::find_existing_container(&project).await.unwrap_or(None);
|
let from_container = match container_id {
|
||||||
let from_container = match &container_id {
|
|
||||||
Some(id) => container_label(id, mig::LABEL_BASE_IMAGE_ID).await,
|
Some(id) => container_label(id, mig::LABEL_BASE_IMAGE_ID).await,
|
||||||
None => None,
|
None => None,
|
||||||
};
|
};
|
||||||
@@ -248,17 +500,19 @@ pub async fn get_container_staleness(
|
|||||||
};
|
};
|
||||||
|
|
||||||
// ── Probes ───────────────────────────────────────────────────────────
|
// ── Probes ───────────────────────────────────────────────────────────
|
||||||
let container_running = match &container_id {
|
let snapshot_exists = &inputs.snapshot_exists;
|
||||||
Some(id) => Some(docker::is_container_running(id).await.unwrap_or(false)),
|
let source = match pick_probe_source(&container, snapshot_exists) {
|
||||||
None => None,
|
Ok(source) => source,
|
||||||
|
// The only reading this decision needed and did not get — see
|
||||||
|
// [`ProbeStart::Report`] for why this is a report and not an `Err`.
|
||||||
|
Err(e) => {
|
||||||
|
out.probe_error = Some(e);
|
||||||
|
return Ok(out);
|
||||||
|
}
|
||||||
};
|
};
|
||||||
let snapshot_exists = docker::image_exists(&snapshot_image).await.unwrap_or(false);
|
let from_manifest = match source {
|
||||||
let from_manifest = match (
|
ProbeSource::RunningContainer(id) => mig::manifest_from_container(id).await,
|
||||||
pick_probe_source(container_running, snapshot_exists),
|
ProbeSource::StoppedContainer(id) => {
|
||||||
&container_id,
|
|
||||||
) {
|
|
||||||
(ProbeSource::RunningContainer, Some(id)) => mig::manifest_from_container(id).await,
|
|
||||||
(ProbeSource::StoppedContainer, Some(id)) => {
|
|
||||||
let busy = crate::project_lock::held(&project_id).is_some();
|
let busy = crate::project_lock::held(&project_id).is_some();
|
||||||
match stopped_probe_policy(busy, snapshot_exists) {
|
match stopped_probe_policy(busy, snapshot_exists) {
|
||||||
StoppedProbe::Commit => {
|
StoppedProbe::Commit => {
|
||||||
@@ -275,7 +529,12 @@ pub async fn get_container_staleness(
|
|||||||
// layer; the snapshot probe allocates nothing) and a
|
// layer; the snapshot probe allocates nothing) and a
|
||||||
// 409 from an operation that claimed the project after
|
// 409 from an operation that claimed the project after
|
||||||
// the check above.
|
// the check above.
|
||||||
Err(e) if snapshot_exists => {
|
// `Ok(true)` specifically: an `image_exists` that
|
||||||
|
// failed has not established that there is anything to
|
||||||
|
// fall back to, and probing a snapshot that may not
|
||||||
|
// exist would replace the commit's real error with a
|
||||||
|
// confusing one.
|
||||||
|
Err(e) if matches!(snapshot_exists, Ok(true)) => {
|
||||||
log::warn!(
|
log::warn!(
|
||||||
"Probing the stopped container for project {} failed ({}) — \
|
"Probing the stopped container for project {} failed ({}) — \
|
||||||
falling back to its snapshot image, which may lag it",
|
falling back to its snapshot image, which may lag it",
|
||||||
@@ -295,15 +554,16 @@ pub async fn get_container_staleness(
|
|||||||
);
|
);
|
||||||
mig::manifest_from_image(&snapshot_image).await
|
mig::manifest_from_image(&snapshot_image).await
|
||||||
}
|
}
|
||||||
StoppedProbe::Defer => Err(PROJECT_BUSY.to_string()),
|
StoppedProbe::Defer(message) => Err(message),
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
(ProbeSource::Snapshot, _) => mig::manifest_from_image(&snapshot_image).await,
|
ProbeSource::Snapshot => mig::manifest_from_image(&snapshot_image).await,
|
||||||
// `container_running` is `Some` exactly when `container_id` is, so the
|
// Reached only when there is genuinely neither a container nor a
|
||||||
// two arms above are the only ones those variants can reach. This arm
|
// snapshot: `ProbeSource` carries the container id in its container
|
||||||
// is `ProbeSource::Nothing` — and now *only* that: it used to also
|
// variants, so a container that exists can no longer fall through to
|
||||||
// swallow every stopped container, which is the bug.
|
// here — which is the bug this arm used to hide, swallowing every
|
||||||
(_, _) => Err(NOTHING_TO_PROBE.to_string()),
|
// stopped container.
|
||||||
|
ProbeSource::Nothing => Err(NOTHING_TO_PROBE.to_string()),
|
||||||
};
|
};
|
||||||
|
|
||||||
let (from_manifest, base_manifest) = match from_manifest {
|
let (from_manifest, base_manifest) = match from_manifest {
|
||||||
@@ -2113,13 +2373,39 @@ mod tests {
|
|||||||
assert_eq!(pick_recorded_lineage(some(""), None), None);
|
assert_eq!(pick_recorded_lineage(some(""), None), None);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The readings as the daemon answered them, all four healthy: no
|
||||||
|
/// container, nothing pulled, no snapshot. Tests override the one reading
|
||||||
|
/// they are about, which keeps it obvious which reading each case is
|
||||||
|
/// actually exercising.
|
||||||
|
fn readings() -> ProbeReadings {
|
||||||
|
ProbeReadings {
|
||||||
|
container_id: Ok(None),
|
||||||
|
container_running: None,
|
||||||
|
base_image_id: Ok(None),
|
||||||
|
snapshot_exists: Ok(false),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn present(running: bool) -> ContainerState {
|
||||||
|
ContainerState::Present {
|
||||||
|
id: "c1".to_string(),
|
||||||
|
running,
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// What every one of the four readings looks like when the socket is gone:
|
||||||
|
/// generic over what it was going to return.
|
||||||
|
fn daemon<T>() -> Result<T, String> {
|
||||||
|
Err("Failed to list containers: connection refused".to_string())
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_stopped_container_is_probed_rather_than_reported_missing() {
|
fn a_stopped_container_is_probed_rather_than_reported_missing() {
|
||||||
// The regression: a container that exists but is stopped, with no
|
// The regression: a container that exists but is stopped, with no
|
||||||
// snapshot ever taken, read as "nothing to compare against".
|
// snapshot ever taken, read as "nothing to compare against".
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
pick_probe_source(Some(false), false),
|
pick_probe_source(&present(false), &Ok(false)),
|
||||||
ProbeSource::StoppedContainer
|
Ok(ProbeSource::StoppedContainer("c1"))
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -2128,42 +2414,283 @@ mod tests {
|
|||||||
// The snapshot lags the container by everything installed since the
|
// The snapshot lags the container by everything installed since the
|
||||||
// last commit, in both states.
|
// last commit, in both states.
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
pick_probe_source(Some(true), true),
|
pick_probe_source(&present(true), &Ok(true)),
|
||||||
ProbeSource::RunningContainer
|
Ok(ProbeSource::RunningContainer("c1"))
|
||||||
);
|
);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
pick_probe_source(Some(false), true),
|
pick_probe_source(&present(false), &Ok(true)),
|
||||||
ProbeSource::StoppedContainer
|
Ok(ProbeSource::StoppedContainer("c1"))
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn the_snapshot_is_the_fallback_only_once_the_container_is_gone() {
|
fn the_snapshot_is_the_fallback_only_once_the_container_is_gone() {
|
||||||
assert_eq!(pick_probe_source(None, true), ProbeSource::Snapshot);
|
assert_eq!(
|
||||||
|
pick_probe_source(&ContainerState::Absent, &Ok(true)),
|
||||||
|
Ok(ProbeSource::Snapshot)
|
||||||
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn nothing_to_probe_is_reserved_for_no_container_and_no_snapshot() {
|
fn nothing_to_probe_is_reserved_for_no_container_and_no_snapshot() {
|
||||||
// The one case the "no container or snapshot image yet" message may
|
// The one case the "no container or snapshot image yet" message may
|
||||||
// still describe.
|
// still describe.
|
||||||
assert_eq!(pick_probe_source(None, false), ProbeSource::Nothing);
|
assert_eq!(
|
||||||
|
pick_probe_source(&ContainerState::Absent, &Ok(false)),
|
||||||
|
Ok(ProbeSource::Nothing)
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn an_unreadable_snapshot_only_costs_the_report_where_the_snapshot_is_the_answer() {
|
||||||
|
// A container answered, so `image_exists` decides nothing: its failure
|
||||||
|
// must not cost a report the container can supply in full. Treating it
|
||||||
|
// as fatal turned "running container, one flaky `image_exists`" into a
|
||||||
|
// bare probe_error with Update disabled.
|
||||||
|
assert_eq!(
|
||||||
|
pick_probe_source(&present(true), &daemon()),
|
||||||
|
Ok(ProbeSource::RunningContainer("c1"))
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
pick_probe_source(&present(false), &daemon()),
|
||||||
|
Ok(ProbeSource::StoppedContainer("c1"))
|
||||||
|
);
|
||||||
|
|
||||||
|
// With no container, the snapshot is the whole decision, so its failure
|
||||||
|
// is reported — and never as "no container or snapshot image yet",
|
||||||
|
// which nothing has established.
|
||||||
|
let e = pick_probe_source(&ContainerState::Absent, &daemon()).unwrap_err();
|
||||||
|
assert!(e.contains("connection refused"), "{}", e);
|
||||||
|
assert_ne!(e, NOTHING_TO_PROBE);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_stopped_container_is_committed_only_when_nothing_else_owns_the_project() {
|
fn a_stopped_container_is_committed_only_when_nothing_else_owns_the_project() {
|
||||||
assert_eq!(stopped_probe_policy(false, false), StoppedProbe::Commit);
|
assert_eq!(
|
||||||
assert_eq!(stopped_probe_policy(false, true), StoppedProbe::Commit);
|
stopped_probe_policy(false, &Ok(false)),
|
||||||
|
StoppedProbe::Commit
|
||||||
|
);
|
||||||
|
assert_eq!(stopped_probe_policy(false, &Ok(true)), StoppedProbe::Commit);
|
||||||
|
// Not the snapshot's business either way when the project is free: an
|
||||||
|
// unreadable `image_exists` does not stop the commit that would not
|
||||||
|
// have consulted it.
|
||||||
|
assert_eq!(stopped_probe_policy(false, &daemon()), StoppedProbe::Commit);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn a_busy_project_falls_back_rather_than_racing_a_recreate() {
|
fn a_busy_project_falls_back_rather_than_racing_a_recreate() {
|
||||||
// The snapshot lags, but a stale answer beats failing someone's Start.
|
// The snapshot lags, but a stale answer beats failing someone's Start.
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
stopped_probe_policy(true, true),
|
stopped_probe_policy(true, &Ok(true)),
|
||||||
StoppedProbe::SnapshotInstead
|
StoppedProbe::SnapshotInstead
|
||||||
);
|
);
|
||||||
// Nothing to fall back to: say so instead of committing anyway.
|
// Nothing to fall back to: say so instead of committing anyway.
|
||||||
assert_eq!(stopped_probe_policy(true, false), StoppedProbe::Defer);
|
assert_eq!(
|
||||||
|
stopped_probe_policy(true, &Ok(false)),
|
||||||
|
StoppedProbe::Defer(PROJECT_BUSY.to_string())
|
||||||
|
);
|
||||||
|
|
||||||
|
// Busy *and* the fallback could not be read: "try again once it
|
||||||
|
// finishes" would promise that waiting is all that stands in the way,
|
||||||
|
// which the failed reading has not established. Report what happened.
|
||||||
|
match stopped_probe_policy(true, &daemon()) {
|
||||||
|
StoppedProbe::Defer(message) => {
|
||||||
|
assert!(message.contains("connection refused"), "{}", message);
|
||||||
|
assert_ne!(message, PROJECT_BUSY);
|
||||||
|
}
|
||||||
|
other => panic!("expected Defer, got {:?}", other),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn an_unreachable_daemon_is_never_read_as_an_absent_container() {
|
||||||
|
// The bug: every one of these used to be flattened to "no" by an
|
||||||
|
// `unwrap_or`, which reached `pick_probe_source` as "no container, no
|
||||||
|
// snapshot" and reported "no container or snapshot image yet" about a
|
||||||
|
// project nobody had managed to look at.
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
container_id: daemon(),
|
||||||
|
..readings()
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(e.contains("connection refused"), "{}", e);
|
||||||
|
assert_ne!(e, NOTHING_TO_PROBE);
|
||||||
|
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
base_image_id: daemon(),
|
||||||
|
..readings()
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(e.contains("connection refused"), "{}", e);
|
||||||
|
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
container_id: Ok(Some("c1".into())),
|
||||||
|
container_running: Some(daemon()),
|
||||||
|
..readings()
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(e.contains("connection refused"), "{}", e);
|
||||||
|
|
||||||
|
// The fourth reading is not fatal here — see
|
||||||
|
// `an_unreadable_snapshot_only_costs_the_report_where_the_snapshot_is_the_answer`
|
||||||
|
// — but it must still arrive as an error rather than as "no snapshot".
|
||||||
|
let inputs = collect_probe_inputs(ProbeReadings {
|
||||||
|
snapshot_exists: daemon(),
|
||||||
|
..readings()
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
assert!(inputs.snapshot_exists.is_err());
|
||||||
|
let e = pick_probe_source(&inputs.container, &inputs.snapshot_exists).unwrap_err();
|
||||||
|
assert_ne!(e, NOTHING_TO_PROBE);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_base_image_that_could_not_be_read_is_never_reported_as_up_to_date() {
|
||||||
|
// `image_id` answers `Ok(None)` for "not pulled locally", which is a
|
||||||
|
// legitimate `stale: false`. An `Err` is not: it is the right-hand side
|
||||||
|
// of the comparison missing, and letting it through as `None` would
|
||||||
|
// report the project up to date on the strength of a reading nobody
|
||||||
|
// got. This is #56 one field over, so it is fatal on purpose.
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
base_image_id: Err("invalid reference format".into()),
|
||||||
|
container_id: Ok(Some("c1".into())),
|
||||||
|
container_running: Some(Ok(true)),
|
||||||
|
snapshot_exists: Ok(true),
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(e.contains("invalid reference format"), "{}", e);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_failed_reading_is_not_blamed_on_a_daemon_that_answered() {
|
||||||
|
// Three of the four readings return `Err` from a daemon that replied
|
||||||
|
// perfectly well: `image_id` maps only a 404 to `Ok(None)`, and the two
|
||||||
|
// list-based readings wrap any failure. The base image name is
|
||||||
|
// user-supplied, so a typo in settings lands here — and used to be
|
||||||
|
// reported as "Docker could not be reached", sending the user to fix a
|
||||||
|
// daemon that was running.
|
||||||
|
// The payload is the shape bollard really produces for this case, and
|
||||||
|
// it contains the word "Docker" itself — so asserting the *message*
|
||||||
|
// lacks that word would pass here only because a synthetic payload was
|
||||||
|
// chosen. What must be true is that nothing *we* add claims the daemon
|
||||||
|
// was unreachable, or names the container when the reading was about
|
||||||
|
// the base image.
|
||||||
|
let raw = "Docker responded with status code 400: invalid reference format";
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
base_image_id: Err(raw.into()),
|
||||||
|
..readings()
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(!e.contains("could not be reached"), "{}", e);
|
||||||
|
assert!(!e.contains("container"), "{}", e);
|
||||||
|
// The cause still comes through verbatim: "Docker isn't running" and
|
||||||
|
// "permission denied on the socket" need different fixes and must stay
|
||||||
|
// distinguishable.
|
||||||
|
assert!(e.contains(raw), "{}", e);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn the_first_daemon_error_is_the_one_reported() {
|
||||||
|
// When the daemon is down these fail together, and the user needs the
|
||||||
|
// reason once rather than three times. Call order wins, and
|
||||||
|
// `container_id` leads because it is what selects the probe source.
|
||||||
|
//
|
||||||
|
// At most three fail, not four: `container_running` is only attempted
|
||||||
|
// when `container_id` answered with a container, so the caller cannot
|
||||||
|
// produce an `Err` container id alongside a `Some(..)` running reading.
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
container_id: Err("first".into()),
|
||||||
|
container_running: None,
|
||||||
|
base_image_id: Err("second".into()),
|
||||||
|
snapshot_exists: Err("third".into()),
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(e.ends_with("first"), "{}", e);
|
||||||
|
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
container_id: Ok(Some("c1".into())),
|
||||||
|
container_running: Some(Err("third".into())),
|
||||||
|
base_image_id: Err("second".into()),
|
||||||
|
snapshot_exists: Err("fourth".into()),
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(e.ends_with("second"), "{}", e);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_container_id_cannot_arrive_without_a_reading_of_its_state() {
|
||||||
|
// `ContainerState` makes "running, but no container" unrepresentable;
|
||||||
|
// this is the other half — a container found, but never asked about.
|
||||||
|
// The caller cannot produce it, and guessing "stopped" would cost a
|
||||||
|
// running project the only probe source that sees this session's
|
||||||
|
// installs.
|
||||||
|
let e = collect_probe_inputs(ProbeReadings {
|
||||||
|
container_id: Ok(Some("c1".into())),
|
||||||
|
container_running: None,
|
||||||
|
..readings()
|
||||||
|
})
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(e.contains("state was not read"), "{}", e);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_daemon_that_answers_no_is_an_answer_and_passes_through() {
|
||||||
|
// No container, no snapshot, base image not pulled: all the readings
|
||||||
|
// are `Ok`, and the "nothing to probe" path downstream is then
|
||||||
|
// genuinely earned.
|
||||||
|
let inputs = collect_probe_inputs(readings()).unwrap();
|
||||||
|
assert_eq!(inputs.current_base_image_id, None);
|
||||||
|
assert_eq!(inputs.container, ContainerState::Absent);
|
||||||
|
assert_eq!(inputs.snapshot_exists, Ok(false));
|
||||||
|
assert_eq!(
|
||||||
|
pick_probe_source(&inputs.container, &inputs.snapshot_exists),
|
||||||
|
Ok(ProbeSource::Nothing)
|
||||||
|
);
|
||||||
|
|
||||||
|
// And the fully populated reading survives intact.
|
||||||
|
let inputs = collect_probe_inputs(ProbeReadings {
|
||||||
|
container_id: Ok(Some("c1".into())),
|
||||||
|
container_running: Some(Ok(true)),
|
||||||
|
base_image_id: Ok(Some("sha256:base".into())),
|
||||||
|
snapshot_exists: Ok(true),
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(inputs.current_base_image_id.as_deref(), Some("sha256:base"));
|
||||||
|
assert_eq!(inputs.container, present(true));
|
||||||
|
assert_eq!(inputs.snapshot_exists, Ok(true));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_failed_reading_keeps_the_banner_on_screen_instead_of_erroring() {
|
||||||
|
// The load-bearing design decision of this path: a failed reading is a
|
||||||
|
// report with `probe_error` set, never an `Err` out of the command. An
|
||||||
|
// `Err` reaches the hook's `catch`, which nulls `staleness`, and
|
||||||
|
// `ContainerMigrationBanner` renders nothing at all for a null one — so
|
||||||
|
// the banner would vanish at exactly the moment it has something to say.
|
||||||
|
match start_probe(ProbeReadings {
|
||||||
|
container_id: daemon(),
|
||||||
|
..readings()
|
||||||
|
}) {
|
||||||
|
ProbeStart::Report(report) => {
|
||||||
|
let message = report.probe_error.clone().expect("probe_error");
|
||||||
|
assert!(message.contains("connection refused"), "{}", message);
|
||||||
|
// Everything else at its default: a field being empty means
|
||||||
|
// "nothing found", and nothing was found because nothing was
|
||||||
|
// read. `stale: false` here is the absence of a claim, which is
|
||||||
|
// only honest because `probe_error` is carrying the reason.
|
||||||
|
assert_eq!(
|
||||||
|
*report,
|
||||||
|
ContainerStaleness {
|
||||||
|
probe_error: Some(message),
|
||||||
|
..Default::default()
|
||||||
|
}
|
||||||
|
);
|
||||||
|
}
|
||||||
|
ProbeStart::Inputs(_) => panic!("a failed reading must not be probed on"),
|
||||||
|
}
|
||||||
|
|
||||||
|
// And a healthy set of readings still goes on to probe.
|
||||||
|
assert!(matches!(start_probe(readings()), ProbeStart::Inputs(_)));
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
@@ -1036,7 +1036,6 @@ fn pending_cleanup_is_stale(recorded_at: &str, now: chrono::DateTime<chrono::Utc
|
|||||||
#[tauri::command]
|
#[tauri::command]
|
||||||
pub async fn update_project(
|
pub async fn update_project(
|
||||||
project: serde_json::Value,
|
project: serde_json::Value,
|
||||||
app_handle: tauri::AppHandle,
|
|
||||||
state: State<'_, AppState>,
|
state: State<'_, AppState>,
|
||||||
) -> Result<Project, String> {
|
) -> Result<Project, String> {
|
||||||
// Taken as raw JSON, then deserialised, for one reason: a secret field that
|
// Taken as raw JSON, then deserialised, for one reason: a secret field that
|
||||||
@@ -1098,46 +1097,57 @@ pub async fn update_project(
|
|||||||
// [`crate::models::validate_env_vars_update`].
|
// [`crate::models::validate_env_vars_update`].
|
||||||
crate::models::validate_env_vars_update(&stored.custom_env_vars, &project.custom_env_vars)?;
|
crate::models::validate_env_vars_update(&stored.custom_env_vars, &project.custom_env_vars)?;
|
||||||
|
|
||||||
project.container_id = stored.container_id;
|
restore_store_owned_fields(&mut project, &stored);
|
||||||
project.status = stored.status;
|
|
||||||
// `browser_view_enabled` is owned by `set_browser_view_enabled` and is
|
|
||||||
// restored here rather than taken from the payload, exactly like
|
|
||||||
// `container_id` and `status` above. The Config tab has no control for it
|
|
||||||
// — the Browser tab's toggle is the only way it ever changes — so the
|
|
||||||
// project object the frontend round-trips carries whatever it was told at
|
|
||||||
// load time and would silently undo a toggle made since. `auth_bridge_enabled`
|
|
||||||
// is different and does arrive through this save: the Config tab edits it,
|
|
||||||
// which is why the reconcile below follows whatever was just persisted.
|
|
||||||
project.browser_view_enabled = stored.browser_view_enabled;
|
|
||||||
project.created_at = stored.created_at;
|
|
||||||
project.updated_at = chrono::Utc::now().to_rfc3339();
|
project.updated_at = chrono::Utc::now().to_rfc3339();
|
||||||
|
|
||||||
store_secrets_for_project(&project, &explicitly_cleared)?;
|
store_secrets_for_project(&project, &explicitly_cleared)?;
|
||||||
let updated = state.projects_store.update(project)?;
|
|
||||||
|
|
||||||
// `auth_bridge_enabled` can arrive through this generic save as well as
|
// Nothing reconciles the *running* auth bridge here any more, and there is
|
||||||
// through `set_auth_bridge_enabled`, so reconcile the running bridge with
|
// nothing left for such a step to do. This command can no longer change
|
||||||
// whatever was just persisted. `start` is idempotent and `stop` is a no-op
|
// `auth_bridge_enabled` at all (see [`restore_store_owned_fields`]), so a
|
||||||
// when nothing is running, so this is safe on every project save.
|
// reconcile could only ever re-assert what was already true. The paths that
|
||||||
if updated.auth_bridge_enabled {
|
// do change it each own their own side effect: `set_auth_bridge_enabled`
|
||||||
if let Some(ref container_id) = updated.container_id {
|
// starts or stops the bridge itself, [`start_project_container`] arms it
|
||||||
if docker::is_container_running(container_id).await.unwrap_or(false) {
|
// when the container comes up, and `reconcile_project_statuses` re-arms it
|
||||||
state
|
// for every already-running container at launch. The version of this that
|
||||||
.auth_bridge
|
// re-asserted on every save is what turned a stale flag in a payload into a
|
||||||
.start(
|
// restarted bridge.
|
||||||
updated.id.clone(),
|
state.projects_store.update(project)
|
||||||
container_id.clone(),
|
}
|
||||||
app_handle,
|
|
||||||
state.projects_store.clone(),
|
|
||||||
)
|
|
||||||
.await;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
} else {
|
|
||||||
state.auth_bridge.stop(&updated.id).await;
|
|
||||||
}
|
|
||||||
|
|
||||||
Ok(updated)
|
/// Restore onto `project` the fields whose value belongs to the store rather
|
||||||
|
/// than to whoever is saving the project. See the comment above `stored` in
|
||||||
|
/// [`update_project`] for `container_id`, `status` and `created_at`.
|
||||||
|
///
|
||||||
|
/// **Both feature flags are in here, for one reason that covers them equally:
|
||||||
|
/// neither ever arrives through this command as an edit.** Each has a
|
||||||
|
/// dedicated setter — [`crate::browser_view::commands::set_browser_view_enabled`]
|
||||||
|
/// and [`crate::commands::auth_bridge_commands::set_auth_bridge_enabled`] —
|
||||||
|
/// and that setter is the only control the UI offers for it. Neither is wired
|
||||||
|
/// into the Config tab's `save`: the browser view's toggle lives in the Browser
|
||||||
|
/// tab, and `AuthBridgeRow`'s switch calls `set_auth_bridge_enabled` directly
|
||||||
|
/// even though it is rendered *in* the Config tab, because that tab's editors
|
||||||
|
/// are disabled while the container runs and the bridge is precisely the thing
|
||||||
|
/// a user needs to flip while a login is hanging.
|
||||||
|
///
|
||||||
|
/// So the flags in an incoming payload are never a choice — they are whatever
|
||||||
|
/// the frontend was told when it loaded the project, and the setters do not
|
||||||
|
/// write their new value back into frontend app state. Every unrelated save
|
||||||
|
/// (a renamed session, an env var, a mount name) carries that snapshot back.
|
||||||
|
/// Taking it would silently undo a toggle made since.
|
||||||
|
///
|
||||||
|
/// This restored only `browser_view_enabled` before, on the stated belief that
|
||||||
|
/// the Config tab edited `auth_bridge_enabled` through this save. It does not.
|
||||||
|
/// The consequence was specific: a user turns the bridge off — having been told
|
||||||
|
/// a bridged port is unauthenticated and reachable by any local process — then
|
||||||
|
/// closes a renamed terminal tab, and the stale `true` in that save re-persisted
|
||||||
|
/// and restarted the bridge.
|
||||||
|
fn restore_store_owned_fields(project: &mut Project, stored: &Project) {
|
||||||
|
project.container_id = stored.container_id.clone();
|
||||||
|
project.status = stored.status.clone();
|
||||||
|
project.browser_view_enabled = stored.browser_view_enabled;
|
||||||
|
project.auth_bridge_enabled = stored.auth_bridge_enabled;
|
||||||
|
project.created_at = stored.created_at.clone();
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tauri::command]
|
#[tauri::command]
|
||||||
@@ -2195,4 +2205,89 @@ mod tests {
|
|||||||
// Changing it to a different root is a change, and refused.
|
// Changing it to a different root is a change, and refused.
|
||||||
assert!(validate_mounted_host_path("x", Some("/"), Some("C:\\")).is_err());
|
assert!(validate_mounted_host_path("x", Some("/"), Some("C:\\")).is_err());
|
||||||
}
|
}
|
||||||
|
// ── Fields a generic save does not get to write ───────────────────────
|
||||||
|
|
||||||
|
/// A project as the store holds it, plus the copy the frontend is about to
|
||||||
|
/// save back: same record, one unrelated edit, and the flags as they were
|
||||||
|
/// when the frontend last loaded it.
|
||||||
|
fn stored_and_stale_payload() -> (Project, Project) {
|
||||||
|
let mut stored = Project::new("demo".to_string(), Vec::new());
|
||||||
|
stored.container_id = Some("abc123".to_string());
|
||||||
|
stored.status = ProjectStatus::Running;
|
||||||
|
|
||||||
|
let mut payload = stored.clone();
|
||||||
|
payload.container_id = None;
|
||||||
|
payload.status = ProjectStatus::Stopped;
|
||||||
|
payload
|
||||||
|
.renamed_session_names
|
||||||
|
.insert("s1".to_string(), "build".to_string());
|
||||||
|
|
||||||
|
(stored, payload)
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The regression. The user turns the auth bridge off — the switch calls
|
||||||
|
/// `set_auth_bridge_enabled`, which persists `false` and stops the bridge,
|
||||||
|
/// and writes nothing back into the frontend's copy of the project. Every
|
||||||
|
/// holder of that copy still has `auth_bridge_enabled: true`, and the next
|
||||||
|
/// unrelated save (closing a renamed terminal tab) posts it back. That save
|
||||||
|
/// must not re-enable the bridge.
|
||||||
|
#[test]
|
||||||
|
fn a_stale_auth_bridge_flag_in_a_save_cannot_re_enable_a_disabled_bridge() {
|
||||||
|
let (mut stored, mut payload) = stored_and_stale_payload();
|
||||||
|
stored.auth_bridge_enabled = false;
|
||||||
|
payload.auth_bridge_enabled = true;
|
||||||
|
|
||||||
|
restore_store_owned_fields(&mut payload, &stored);
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
!payload.auth_bridge_enabled,
|
||||||
|
"a save must not be able to turn the bridge back on: the stored value is the user's"
|
||||||
|
);
|
||||||
|
// The edit the save was actually for still goes through.
|
||||||
|
assert_eq!(
|
||||||
|
payload.renamed_session_names.get("s1").map(String::as_str),
|
||||||
|
Some("build")
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The mirror image, and the reason the serde default going to `true`
|
||||||
|
/// made this worse: a pre-existing record with no `auth_bridge_enabled`
|
||||||
|
/// key reads as enabled, so the stale payload is `true` for every project
|
||||||
|
/// that predates the field. A user who has *not* turned the bridge off is
|
||||||
|
/// equally entitled to have the store's answer win.
|
||||||
|
#[test]
|
||||||
|
fn an_enabled_bridge_is_left_enabled_by_the_same_rule() {
|
||||||
|
let (mut stored, mut payload) = stored_and_stale_payload();
|
||||||
|
stored.auth_bridge_enabled = true;
|
||||||
|
payload.auth_bridge_enabled = false;
|
||||||
|
|
||||||
|
restore_store_owned_fields(&mut payload, &stored);
|
||||||
|
|
||||||
|
assert!(payload.auth_bridge_enabled);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The flag that was already restored, kept under test beside the one that
|
||||||
|
/// was not — the two are owned by their setters for the same reason and
|
||||||
|
/// must not drift apart again.
|
||||||
|
#[test]
|
||||||
|
fn a_stale_browser_view_flag_cannot_undo_the_panes_toggle_either() {
|
||||||
|
let (mut stored, mut payload) = stored_and_stale_payload();
|
||||||
|
stored.browser_view_enabled = true;
|
||||||
|
payload.browser_view_enabled = false;
|
||||||
|
|
||||||
|
restore_store_owned_fields(&mut payload, &stored);
|
||||||
|
|
||||||
|
assert!(payload.browser_view_enabled);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn the_container_handle_status_and_creation_time_still_come_from_the_store() {
|
||||||
|
let (stored, mut payload) = stored_and_stale_payload();
|
||||||
|
|
||||||
|
restore_store_owned_fields(&mut payload, &stored);
|
||||||
|
|
||||||
|
assert_eq!(payload.container_id.as_deref(), Some("abc123"));
|
||||||
|
assert_eq!(payload.status, ProjectStatus::Running);
|
||||||
|
assert_eq!(payload.created_at, stored.created_at);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -346,11 +346,47 @@ const OPENERS: &[(&str, &[&str])] = &[("xdg-open", &[]), ("gio", &["open"])];
|
|||||||
/// `xdg-open` usually returns immediately (it hands the URL to a running
|
/// `xdg-open` usually returns immediately (it hands the URL to a running
|
||||||
/// browser and exits), but in its generic fallback mode it *is* the browser's
|
/// browser and exits), but in its generic fallback mode it *is* the browser's
|
||||||
/// parent and stays alive for the session. So "still running" cannot be read
|
/// parent and stays alive for the session. So "still running" cannot be read
|
||||||
/// as failure, and "exited non-zero quickly" is the only reliable signal
|
/// as failure, and "exited non-zero quickly" is the only negative signal there
|
||||||
/// there is.
|
/// is — though not, on its own, a trustworthy one. See
|
||||||
|
/// [`exit_code_means_nothing_was_launched`].
|
||||||
#[cfg(target_os = "linux")]
|
#[cfg(target_os = "linux")]
|
||||||
const OPENER_GRACE: std::time::Duration = std::time::Duration::from_millis(400);
|
const OPENER_GRACE: std::time::Duration = std::time::Duration::from_millis(400);
|
||||||
|
|
||||||
|
/// Whether a non-zero exit says the opener certainly launched nothing, and so
|
||||||
|
/// that the next candidate can be tried without risking a second tab.
|
||||||
|
///
|
||||||
|
/// The loop used to treat every quick non-zero exit as "it did nothing" and
|
||||||
|
/// fall through. That is safe for most of `xdg-open`'s documented codes — 1
|
||||||
|
/// (syntax), 2 (file not found) and 3 (a required tool could not be found) are
|
||||||
|
/// all statements that it never got as far as launching a handler, and 3 is the
|
||||||
|
/// missing-association case `gio open` is in [`OPENERS`] for. 127 is the same
|
||||||
|
/// statement made by a shell, which is how a `$BROWSER` or `x-www-browser`
|
||||||
|
/// wrapper naming a program that does not exist comes back.
|
||||||
|
///
|
||||||
|
/// Code 4 is the one that cannot be read that way, and it is the catch-all:
|
||||||
|
/// "the action failed" also covers a handler that *was* launched and then
|
||||||
|
/// returned non-zero. A browser that takes the URL, opens the tab in an already
|
||||||
|
/// running instance and exits non-zero for its own reasons ends up here, as
|
||||||
|
/// does a wrapper script that does its job and then returns the exit status of
|
||||||
|
/// something else. Falling through on that hands the same URL to a second
|
||||||
|
/// opener: two tabs for one click, and for an OAuth link two authorize
|
||||||
|
/// requests.
|
||||||
|
///
|
||||||
|
/// So anything not recognised below — 4, an unfamiliar code, or a death by
|
||||||
|
/// signal (`code()` is `None`) — ends the loop rather than continuing it. The
|
||||||
|
/// caller is told the opener failed, which is the honest report of an
|
||||||
|
/// ambiguous outcome, and no second request is made on the user's behalf. Note
|
||||||
|
/// what this costs: an opener that genuinely failed with code 4 no longer falls
|
||||||
|
/// through to `gio`, so a user whose `xdg-open` fails that way sees an error
|
||||||
|
/// where they previously might have got a tab.
|
||||||
|
///
|
||||||
|
/// This is reasoning from `xdg-open`'s documented exit codes, not from an
|
||||||
|
/// observed double-open in this app.
|
||||||
|
#[cfg(target_os = "linux")]
|
||||||
|
fn exit_code_means_nothing_was_launched(code: Option<i32>) -> bool {
|
||||||
|
matches!(code, Some(1 | 2 | 3 | 127))
|
||||||
|
}
|
||||||
|
|
||||||
/// Spawn `url` with an opener, under a sanitized environment.
|
/// Spawn `url` with an opener, under a sanitized environment.
|
||||||
#[cfg(target_os = "linux")]
|
#[cfg(target_os = "linux")]
|
||||||
fn spawn_with_clean_env(url: &str) -> Result<(), String> {
|
fn spawn_with_clean_env(url: &str) -> Result<(), String> {
|
||||||
@@ -383,6 +419,10 @@ fn spawn_with_clean_env(url: &str) -> Result<(), String> {
|
|||||||
.stdout(std::process::Stdio::null())
|
.stdout(std::process::Stdio::null())
|
||||||
.stderr(std::process::Stdio::null());
|
.stderr(std::process::Stdio::null());
|
||||||
|
|
||||||
|
// A spawn failure — `ErrorKind::NotFound` for an opener that is not
|
||||||
|
// installed, `PermissionDenied` for one that cannot be executed — is
|
||||||
|
// the unambiguous case: nothing ran, so nothing was opened, and the
|
||||||
|
// next candidate is free to try.
|
||||||
let mut child = match command.spawn() {
|
let mut child = match command.spawn() {
|
||||||
Ok(child) => child,
|
Ok(child) => child,
|
||||||
Err(err) => {
|
Err(err) => {
|
||||||
@@ -395,6 +435,14 @@ fn spawn_with_clean_env(url: &str) -> Result<(), String> {
|
|||||||
match child.try_wait() {
|
match child.try_wait() {
|
||||||
Ok(Some(status)) if !status.success() => {
|
Ok(Some(status)) if !status.success() => {
|
||||||
failures.push(format!("{program} exited with {status}"));
|
failures.push(format!("{program} exited with {status}"));
|
||||||
|
// A program that *ran* is not a program that did nothing.
|
||||||
|
if !exit_code_means_nothing_was_launched(status.code()) {
|
||||||
|
return Err(format!(
|
||||||
|
"Could not confirm the link opened. Tried: {}. It may have opened anyway \
|
||||||
|
— check your browser before trying again.",
|
||||||
|
failures.join("; ")
|
||||||
|
));
|
||||||
|
}
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
Ok(_) => {}
|
Ok(_) => {}
|
||||||
@@ -699,3 +747,45 @@ mod tests {
|
|||||||
assert_eq!(changes, vec![("GTK_PATH".to_string(), None)]);
|
assert_eq!(changes, vec![("GTK_PATH".to_string(), None)]);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(all(test, target_os = "linux"))]
|
||||||
|
mod opener_fallback_tests {
|
||||||
|
use super::*;
|
||||||
|
|
||||||
|
/// The codes `xdg-open` documents as "nothing was launched". Falling
|
||||||
|
/// through to the next opener on these is what keeps `gio open` reachable
|
||||||
|
/// for the case it was added for: no usable `x-scheme-handler/https`
|
||||||
|
/// association.
|
||||||
|
#[test]
|
||||||
|
fn the_codes_that_mean_no_handler_ran_fall_through() {
|
||||||
|
for code in [1, 2, 3, 127] {
|
||||||
|
assert!(
|
||||||
|
exit_code_means_nothing_was_launched(Some(code)),
|
||||||
|
"exit {code} means the opener never launched anything"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The regression this guards: `xdg-open` returns 4 both when it could not
|
||||||
|
/// act and when the handler it launched returned non-zero — including a
|
||||||
|
/// browser that had already opened the tab. Trying `gio open` next would
|
||||||
|
/// open it a second time, which for an OAuth URL is a second authorize
|
||||||
|
/// request.
|
||||||
|
#[test]
|
||||||
|
fn an_exit_that_may_follow_a_successful_open_does_not_fall_through() {
|
||||||
|
assert!(!exit_code_means_nothing_was_launched(Some(4)));
|
||||||
|
for code in [5, 7, 126, 255] {
|
||||||
|
assert!(
|
||||||
|
!exit_code_means_nothing_was_launched(Some(code)),
|
||||||
|
"exit {code} is not a documented 'did nothing', so it must not be assumed to be one"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Killed by a signal: `code()` is `None` and the outcome is unknowable,
|
||||||
|
/// so it is treated like any other unrecognised exit.
|
||||||
|
#[test]
|
||||||
|
fn a_death_by_signal_does_not_fall_through() {
|
||||||
|
assert!(!exit_code_means_nothing_was_launched(None));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -626,7 +626,7 @@ describe("chooseSignInTarget — which action leads for a sign-in link", () => {
|
|||||||
// and the container-side target is Playwright's pane, whose browsers are not
|
// and the container-side target is Playwright's pane, whose browsers are not
|
||||||
// in the image.
|
// in the image.
|
||||||
it("prefers the host browser whenever the bridge is live", () => {
|
it("prefers the host browser whenever the bridge is live", () => {
|
||||||
expect(chooseSignInTarget(LIVE_BRIDGE, usableDetection())).toBe("host");
|
expect(chooseSignInTarget(LIVE_BRIDGE, usableDetection())).toBe("host-bridged");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("does not call a bridge live while it is holding a port conflict", () => {
|
it("does not call a bridge live while it is holding a port conflict", () => {
|
||||||
@@ -644,13 +644,17 @@ describe("chooseSignInTarget — which action leads for a sign-in link", () => {
|
|||||||
// There is nothing to bridge until the CLI binds its listener, and that
|
// There is nothing to bridge until the CLI binds its listener, and that
|
||||||
// races the URL reaching the transcript. Requiring a port would make the
|
// races the URL reaching the transcript. Requiring a port would make the
|
||||||
// default flip between two identical sign-ins.
|
// default flip between two identical sign-ins.
|
||||||
expect(chooseSignInTarget(LIVE_BRIDGE, null)).toBe("host");
|
expect(chooseSignInTarget(LIVE_BRIDGE, null)).toBe("host-bridged");
|
||||||
});
|
});
|
||||||
|
|
||||||
it("falls to the container only when it has a browser to open", () => {
|
it("falls to the container only when it has a browser to open", () => {
|
||||||
const off: AuthBridgeStatus = { enabled: false, active_ports: [], conflicts: [] };
|
const off: AuthBridgeStatus = { enabled: false, active_ports: [], conflicts: [] };
|
||||||
expect(chooseSignInTarget(off, usableDetection())).toBe("container");
|
expect(chooseSignInTarget(off, usableDetection())).toBe("container");
|
||||||
expect(chooseSignInTarget(off, null)).toBe("host");
|
// Not plain "host": with the bridge off and no browser inside, nothing is
|
||||||
|
// carrying the callback, and the toast's hint has to say so rather than
|
||||||
|
// promising a bridge. That distinction is the whole reason this answer is
|
||||||
|
// three-valued.
|
||||||
|
expect(chooseSignInTarget(off, null)).toBe("host-fallback");
|
||||||
// Packages installed, cache empty — the fresh-project state, and the one
|
// Packages installed, cache empty — the fresh-project state, and the one
|
||||||
// that used to be the silent default.
|
// that used to be the silent default.
|
||||||
expect(
|
expect(
|
||||||
@@ -658,13 +662,27 @@ describe("chooseSignInTarget — which action leads for a sign-in link", () => {
|
|||||||
off,
|
off,
|
||||||
usableDetection({ browsers: [], chromium_executable_exists: false }),
|
usableDetection({ browsers: [], chromium_executable_exists: false }),
|
||||||
),
|
),
|
||||||
).toBe("host");
|
).toBe("host-fallback");
|
||||||
// Playwright too old to bind: the pane cannot show it either.
|
// Playwright too old to bind: the pane cannot show it either.
|
||||||
expect(chooseSignInTarget(off, usableDetection({ has_bind: false }))).toBe("host");
|
expect(chooseSignInTarget(off, usableDetection({ has_bind: false }))).toBe(
|
||||||
|
"host-fallback",
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("answers host when nothing is known at all", () => {
|
it("answers the host *fallback* when nothing is known at all", () => {
|
||||||
expect(chooseSignInTarget(null, null)).toBe("host");
|
// "Unknown" must not read as "bridged". A status call that never answered
|
||||||
|
// is not evidence that something will carry the callback home.
|
||||||
|
expect(chooseSignInTarget(null, null)).toBe("host-fallback");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("separates a live bridge from the least-bad answer, though both lead with the host", () => {
|
||||||
|
const off: AuthBridgeStatus = { enabled: false, active_ports: [], conflicts: [] };
|
||||||
|
// The two states the old two-valued answer collapsed together. Folding them
|
||||||
|
// back into one is what let the toast tell a user with the bridge disabled
|
||||||
|
// that the bridge would carry their callback.
|
||||||
|
expect(chooseSignInTarget(LIVE_BRIDGE, null)).not.toBe(
|
||||||
|
chooseSignInTarget(off, null),
|
||||||
|
);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -726,6 +744,26 @@ describe("TerminalView — the sign-in default follows the project", () => {
|
|||||||
await mountWithPrompt();
|
await mountWithPrompt();
|
||||||
expect(primaryLabel()).toBe("Open");
|
expect(primaryLabel()).toBe("Open");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not promise the auth bridge on a project that has it switched off", async () => {
|
||||||
|
// The end-to-end version of the three-state answer: bridge off, no browser
|
||||||
|
// inside. The host still leads, because it is the least bad of two answers
|
||||||
|
// that can both fail — but the hint must not tell the user the bridge is
|
||||||
|
// bringing their callback home, because there is no bridge. That hint is
|
||||||
|
// what sent people to a host browser and a login that hung to its timeout.
|
||||||
|
await mountWithPrompt();
|
||||||
|
const hint = document.querySelector('[data-testid="url-toast-signin-hint"]');
|
||||||
|
expect(hint?.textContent).toMatch(/nothing is set up/i);
|
||||||
|
expect(hint?.textContent).not.toMatch(/what carries the callback/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does promise it when the bridge is actually live", async () => {
|
||||||
|
containerEnv.bridge = LIVE_BRIDGE;
|
||||||
|
await mountWithPrompt();
|
||||||
|
const hint = document.querySelector('[data-testid="url-toast-signin-hint"]');
|
||||||
|
expect(hint?.textContent).toMatch(/auth bridge/i);
|
||||||
|
expect(hint?.textContent).not.toMatch(/nothing is set up/i);
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("TerminalView — a host open that fails says so", () => {
|
describe("TerminalView — a host open that fails says so", () => {
|
||||||
@@ -794,6 +832,108 @@ describe("TerminalView — a host open that fails says so", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("TerminalView — an open in flight must not blank a newer prompt", () => {
|
||||||
|
// The window is real and is measured in hundreds of milliseconds, not in
|
||||||
|
// microtasks: on Linux the opener sleeps `OPENER_GRACE` (400 ms, doubled when
|
||||||
|
// `xdg-open` fails and `gio` is tried) before resolving. The container is free
|
||||||
|
// to relay a second URL inside it — a `gh auth login` right after a
|
||||||
|
// `claude login` is the ordinary way that happens — and the toast slot is
|
||||||
|
// shared, so by the time the first open answers the slot may be holding a
|
||||||
|
// prompt the user has never seen. Blanking it loses that URL for good: it
|
||||||
|
// exists nowhere but the container's transcript.
|
||||||
|
const URL_A = "https://github.com/login/device?code=AAAA-1111";
|
||||||
|
const URL_B = "https://claude.ai/oauth/authorize?code=true&client_id=b";
|
||||||
|
|
||||||
|
function relaySequence(url: string): number[] {
|
||||||
|
return Array.from(
|
||||||
|
new TextEncoder().encode(`\x1b]7777;open;${btoa(url)}\x07`),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
async function emitRelay(url: string) {
|
||||||
|
const emit = ptyOutput.listeners.get("terminal-output-s1");
|
||||||
|
if (!emit) throw new Error("no terminal-output listener registered");
|
||||||
|
await act(async () => {
|
||||||
|
emit({ payload: relaySequence(url) });
|
||||||
|
await new Promise((r) => setTimeout(r, 0));
|
||||||
|
await new Promise((r) => setTimeout(r, 0));
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
function openButton(): HTMLElement {
|
||||||
|
const el = Array.from(document.querySelectorAll("button")).find(
|
||||||
|
(b) => b.textContent === "Open",
|
||||||
|
);
|
||||||
|
if (!el) throw new Error("Open button not found");
|
||||||
|
return el as HTMLElement;
|
||||||
|
}
|
||||||
|
|
||||||
|
function promptedUrl(): string | null {
|
||||||
|
return (
|
||||||
|
document
|
||||||
|
.querySelector('[data-testid="url-toast-url"]')
|
||||||
|
?.getAttribute("title") ?? null
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
/** An `openUrlExternal` that hangs until the test lets it finish. */
|
||||||
|
function deferredOpen(): () => void {
|
||||||
|
let finish: () => void = () => {};
|
||||||
|
vi.mocked(openUrlExternal).mockReturnValueOnce(
|
||||||
|
new Promise<void>((resolve) => {
|
||||||
|
finish = () => resolve();
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
return () => finish();
|
||||||
|
}
|
||||||
|
|
||||||
|
it("keeps URL B's prompt when A's open resolves after B arrived", async () => {
|
||||||
|
const finishOpen = deferredOpen();
|
||||||
|
mountSession("claude");
|
||||||
|
await act(async () => {});
|
||||||
|
await emitRelay(URL_A);
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
fireEvent.click(openButton());
|
||||||
|
});
|
||||||
|
expect(openUrlExternal).toHaveBeenCalledWith(URL_A);
|
||||||
|
|
||||||
|
// The container supersedes it while the opener is still inside its grace.
|
||||||
|
await emitRelay(URL_B);
|
||||||
|
expect(promptedUrl()).toBe(URL_B);
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
finishOpen();
|
||||||
|
await Promise.resolve();
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
|
||||||
|
expect(document.querySelector(URL_TOAST_SELECTOR)).not.toBeNull();
|
||||||
|
expect(promptedUrl()).toBe(URL_B);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still dismisses when the slot is holding the prompt that was opened", async () => {
|
||||||
|
// The other half of the guard: it must not turn "dismiss on success" into
|
||||||
|
// "never dismiss". Same deferred open, nothing superseding it.
|
||||||
|
const finishOpen = deferredOpen();
|
||||||
|
mountSession("claude");
|
||||||
|
await act(async () => {});
|
||||||
|
await emitRelay(URL_A);
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
fireEvent.click(openButton());
|
||||||
|
});
|
||||||
|
expect(document.querySelector(URL_TOAST_SELECTOR)).not.toBeNull();
|
||||||
|
|
||||||
|
await act(async () => {
|
||||||
|
finishOpen();
|
||||||
|
await Promise.resolve();
|
||||||
|
await Promise.resolve();
|
||||||
|
});
|
||||||
|
expect(document.querySelector(URL_TOAST_SELECTOR)).toBeNull();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
describe("TerminalView — focus on request", () => {
|
describe("TerminalView — focus on request", () => {
|
||||||
/** Mount, then deliberately give focus away, so what the assertions below
|
/** Mount, then deliberately give focus away, so what the assertions below
|
||||||
* observe is the *request* taking effect and never the focus `active`
|
* observe is the *request* taking effect and never the focus `active`
|
||||||
|
|||||||
@@ -49,6 +49,22 @@ interface Props {
|
|||||||
*/
|
*/
|
||||||
export type PromptSource = "relay" | UrlSource;
|
export type PromptSource = "relay" | UrlSource;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What the shared prompt slot holds.
|
||||||
|
*
|
||||||
|
* `seq` is identity: the slot is one long-lived place that several prompts pass
|
||||||
|
* through, so "is this still the prompt I acted on?" cannot be answered by the
|
||||||
|
* URL (the same link can legitimately be relayed twice) and must not be
|
||||||
|
* answered by "is anything there?". It keys the toast for remounting *and*
|
||||||
|
* guards the deferred dismissal — see `dismissUrlPromptIfCurrent`.
|
||||||
|
*/
|
||||||
|
interface UrlPrompt {
|
||||||
|
url: string;
|
||||||
|
label: string;
|
||||||
|
source: PromptSource;
|
||||||
|
seq: number;
|
||||||
|
}
|
||||||
|
|
||||||
/** Higher wins. Provenance, not recency. */
|
/** Higher wins. Provenance, not recency. */
|
||||||
const SOURCE_RANK: Record<PromptSource, number> = {
|
const SOURCE_RANK: Record<PromptSource, number> = {
|
||||||
heuristic: 0,
|
heuristic: 0,
|
||||||
@@ -132,17 +148,22 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
// replacing a first would otherwise mutate the toast in place, swapping the
|
// replacing a first would otherwise mutate the toast in place, swapping the
|
||||||
// text under a user who is mid-read and mid-click. Keying the toast on it
|
// text under a user who is mid-read and mid-click. Keying the toast on it
|
||||||
// remounts the component, so a new URL is unmistakably a new prompt.
|
// remounts the component, so a new URL is unmistakably a new prompt.
|
||||||
const [urlPrompt, setUrlPrompt] = useState<{
|
const [urlPrompt, setUrlPrompt] = useState<UrlPrompt | null>(null);
|
||||||
url: string;
|
|
||||||
label: string;
|
|
||||||
source: PromptSource;
|
|
||||||
seq: number;
|
|
||||||
} | null>(null);
|
|
||||||
const promptSeqRef = useRef(0);
|
const promptSeqRef = useRef(0);
|
||||||
const relayLimiterRef = useRef(new RelayRateLimiter());
|
const relayLimiterRef = useRef(new RelayRateLimiter());
|
||||||
// Read by the long-lived keyboard listener below, which is registered once
|
/**
|
||||||
// and would otherwise close over the prompt as it was at mount.
|
* A mirror of the prompt slot, written *eagerly* by the two functions that
|
||||||
const urlPromptRef = useRef<{ url: string } | null>(null);
|
* change it.
|
||||||
|
*
|
||||||
|
* Read by the long-lived keyboard listener below, which is registered once
|
||||||
|
* and would otherwise close over the prompt as it was at mount — and by
|
||||||
|
* {@link dismissUrlPromptIfCurrent}, which is the reason it is written on the
|
||||||
|
* spot rather than from an effect. An effect-synced mirror lags the state it
|
||||||
|
* mirrors by a commit, and the whole question that identity check answers is
|
||||||
|
* "did a new prompt land while I was awaiting?" — a mirror that has not
|
||||||
|
* caught up yet answers it wrong in exactly the window that matters.
|
||||||
|
*/
|
||||||
|
const urlPromptRef = useRef<UrlPrompt | null>(null);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Empty the prompt slot, and put focus somewhere real if it was inside the
|
* Empty the prompt slot, and put focus somewhere real if it was inside the
|
||||||
@@ -157,10 +178,40 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
*/
|
*/
|
||||||
const dismissUrlPrompt = useCallback(() => {
|
const dismissUrlPrompt = useCallback(() => {
|
||||||
const wasInside = !!document.activeElement?.closest(URL_TOAST_SELECTOR);
|
const wasInside = !!document.activeElement?.closest(URL_TOAST_SELECTOR);
|
||||||
|
urlPromptRef.current = null;
|
||||||
setUrlPrompt(null);
|
setUrlPrompt(null);
|
||||||
if (wasInside) termRef.current?.focus();
|
if (wasInside) termRef.current?.focus();
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Dismiss, but only if the slot is still holding the prompt the caller
|
||||||
|
* acted on.
|
||||||
|
*
|
||||||
|
* For anything that dismisses *after* awaiting. `openUrlExternal` takes at
|
||||||
|
* least `OPENER_GRACE` (400 ms, doubled when `xdg-open` fails and `gio` is
|
||||||
|
* tried) on Linux by construction, and the container can relay a second,
|
||||||
|
* superseding URL inside that window — at which point the slot has been
|
||||||
|
* remounted with prompt B and an unconditional `setUrlPrompt(null)` blanks
|
||||||
|
* it. The user never sees B, and B exists nowhere but the container's
|
||||||
|
* transcript, which is the exact failure "dismiss on success only" was
|
||||||
|
* introduced to prevent.
|
||||||
|
*
|
||||||
|
* This is a sibling of {@link dismissUrlPrompt} rather than an optional
|
||||||
|
* `expectedSeq` parameter on it, because `dismissUrlPrompt` is handed
|
||||||
|
* straight to `onClick`/`onDismiss`: React would call it with a `MouseEvent`
|
||||||
|
* as its first argument, that event would land in `expectedSeq`, and the ✕
|
||||||
|
* button would silently stop dismissing anything. A parameter that is only
|
||||||
|
* ever correct when nobody passes it by reference is not a safe signature
|
||||||
|
* here.
|
||||||
|
*/
|
||||||
|
const dismissUrlPromptIfCurrent = useCallback(
|
||||||
|
(seq: number) => {
|
||||||
|
if (urlPromptRef.current?.seq !== seq) return;
|
||||||
|
dismissUrlPrompt();
|
||||||
|
},
|
||||||
|
[dismissUrlPrompt],
|
||||||
|
);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The only writer of the prompt slot. Re-validates whatever the caller
|
* The only writer of the prompt slot. Re-validates whatever the caller
|
||||||
* found: the OSC relay branch has already been through `parseUrlRelayOsc`,
|
* found: the OSC relay branch has already been through `parseUrlRelayOsc`,
|
||||||
@@ -179,17 +230,19 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
console.warn("Refusing to prompt for a URL that failed validation");
|
console.warn("Refusing to prompt for a URL that failed validation");
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
setUrlPrompt((current) => {
|
// Read and written through the ref rather than a functional update, so
|
||||||
if (!supersedes({ url, source }, current)) return current;
|
// the mirror is current the instant this returns. Two prompts arriving in
|
||||||
promptSeqRef.current += 1;
|
// one tick still see each other — that is what the ref being the eager
|
||||||
return { url, label, source, seq: promptSeqRef.current };
|
// copy buys — and the seq counter no longer advances inside a state
|
||||||
});
|
// updater, which React is free to run twice.
|
||||||
|
if (!supersedes({ url, source }, urlPromptRef.current)) return;
|
||||||
|
promptSeqRef.current += 1;
|
||||||
|
const next: UrlPrompt = { url, label, source, seq: promptSeqRef.current };
|
||||||
|
urlPromptRef.current = next;
|
||||||
|
setUrlPrompt(next);
|
||||||
},
|
},
|
||||||
[],
|
[],
|
||||||
);
|
);
|
||||||
useEffect(() => {
|
|
||||||
urlPromptRef.current = urlPrompt;
|
|
||||||
}, [urlPrompt]);
|
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The keyboard route into the toast.
|
* The keyboard route into the toast.
|
||||||
@@ -803,11 +856,16 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
*
|
*
|
||||||
* Two things here are ordering, not decoration:
|
* Two things here are ordering, not decoration:
|
||||||
*
|
*
|
||||||
* - **The toast is dismissed on success only.** It used to go first, so a
|
* - **The toast is dismissed on success only, and only if it is still the
|
||||||
* failed open left the user with an empty screen and no way back to a URL
|
* same toast.** Dismissing first is what this replaced: a failed open left
|
||||||
* that only exists in the container's transcript. Now a failure keeps the
|
* the user with an empty screen and no way back to a URL that only exists
|
||||||
* prompt exactly where it was, which also leaves "In container" one click
|
* in the container's transcript. Now a failure keeps the prompt exactly
|
||||||
* away — the fallback this failure is the argument for.
|
* where it was, which also leaves "In container" one click away — the
|
||||||
|
* fallback this failure is the argument for. Waiting to dismiss opens a
|
||||||
|
* second window, though: the open is awaited, the container can relay a
|
||||||
|
* superseding URL while it is in flight, and blanking the slot on success
|
||||||
|
* would then throw away a prompt the user has never seen. Hence the seq
|
||||||
|
* check in `dismissUrlPromptIfCurrent` rather than a bare dismissal.
|
||||||
* - **The failure is a toast, not a `console.error`.** Same `pushToast` the
|
* - **The failure is a toast, not a `console.error`.** Same `pushToast` the
|
||||||
* container-browser branch below uses, because from the user's side the
|
* container-browser branch below uses, because from the user's side the
|
||||||
* two actions fail identically: nothing happens.
|
* two actions fail identically: nothing happens.
|
||||||
@@ -829,8 +887,11 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
dismissUrlPrompt();
|
dismissUrlPrompt();
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
// The prompt this click was for. Captured before the await, because the
|
||||||
|
// slot may be holding a different one by the time the opener answers.
|
||||||
|
const openedSeq = urlPrompt.seq;
|
||||||
openUrlExternal(safe)
|
openUrlExternal(safe)
|
||||||
.then(() => dismissUrlPrompt())
|
.then(() => dismissUrlPromptIfCurrent(openedSeq))
|
||||||
.catch((e) =>
|
.catch((e) =>
|
||||||
useAppState.getState().pushToast({
|
useAppState.getState().pushToast({
|
||||||
kind: "error",
|
kind: "error",
|
||||||
@@ -839,7 +900,7 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
dedupeKey: "host-open-failed",
|
dedupeKey: "host-open-failed",
|
||||||
}),
|
}),
|
||||||
);
|
);
|
||||||
}, [urlPrompt, dismissUrlPrompt]);
|
}, [urlPrompt, dismissUrlPrompt, dismissUrlPromptIfCurrent]);
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Which action leads when the prompt is holding an Anthropic sign-in link.
|
* Which action leads when the prompt is holding an Anthropic sign-in link.
|
||||||
@@ -861,6 +922,13 @@ export default function TerminalView({ sessionId, active }: Props) {
|
|||||||
const handleOpenUrlInContainer = useCallback(() => {
|
const handleOpenUrlInContainer = useCallback(() => {
|
||||||
if (!urlPrompt) return;
|
if (!urlPrompt) return;
|
||||||
const safe = sanitizeRelayUrl(urlPrompt.url);
|
const safe = sanitizeRelayUrl(urlPrompt.url);
|
||||||
|
// Unconditional, and it needs no seq guard, because it happens *before* the
|
||||||
|
// first await: nothing else can have touched the slot between the click and
|
||||||
|
// this line. The success and failure reports below are toasts rather than
|
||||||
|
// this prompt coming back, so there is nothing here that has to survive the
|
||||||
|
// round trip — which is what makes dismissing up front correct here and
|
||||||
|
// wrong in `handleOpenUrl`. Anything that moves this dismissal after the
|
||||||
|
// `openPageInContainerBrowser` call has to take the seq with it.
|
||||||
dismissUrlPrompt();
|
dismissUrlPrompt();
|
||||||
if (!safe) {
|
if (!safe) {
|
||||||
console.warn("Refusing to open a URL that failed validation");
|
console.warn("Refusing to open a URL that failed validation");
|
||||||
|
|||||||
@@ -199,15 +199,14 @@ describe("UrlToast", () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("leads with the host when the caller says so, without hiding the other", () => {
|
it("leads with the host, and promises the bridge, when the bridge is live", () => {
|
||||||
// A live auth bridge, or a container with no browser installed. The pair
|
// The pair is unchanged; only the order and which one is filled.
|
||||||
// is unchanged; only the order and which one is filled.
|
|
||||||
render(
|
render(
|
||||||
<UrlToast
|
<UrlToast
|
||||||
url={SIGN_IN}
|
url={SIGN_IN}
|
||||||
onOpen={noop}
|
onOpen={noop}
|
||||||
onOpenInContainer={noop}
|
onOpenInContainer={noop}
|
||||||
signInDefault="host"
|
signInDefault="host-bridged"
|
||||||
onDismiss={noop}
|
onDismiss={noop}
|
||||||
/>,
|
/>,
|
||||||
);
|
);
|
||||||
@@ -215,15 +214,46 @@ describe("UrlToast", () => {
|
|||||||
expect(
|
expect(
|
||||||
document.querySelector(URL_TOAST_PRIMARY_SELECTOR),
|
document.querySelector(URL_TOAST_PRIMARY_SELECTOR),
|
||||||
).toHaveTextContent("Open");
|
).toHaveTextContent("Open");
|
||||||
// Still recognised as a sign-in, so the explanation stays.
|
// Still recognised as a sign-in, so the explanation stays — and here the
|
||||||
|
// explanation is true, which is the only state in which it may be given.
|
||||||
expect(screen.getByTestId("url-toast-signin-hint")).toHaveTextContent(
|
expect(screen.getByTestId("url-toast-signin-hint")).toHaveTextContent(
|
||||||
/auth bridge/i,
|
/the auth bridge is what carries the callback/i,
|
||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("defaults to the host when the caller passes nothing", () => {
|
it("says the callback has nothing carrying it when the host is the last resort", () => {
|
||||||
// The safe fallback: the answer more likely to work, and the one that
|
// `host-fallback`: bridge off or unknown *and* no browser in the
|
||||||
// reports its own failure.
|
// container. The old two-state hint said the auth bridge would carry the
|
||||||
|
// callback here too, which is a false promise — the user opens the link
|
||||||
|
// in their own browser and `claude login` hangs to its timeout with
|
||||||
|
// nothing on screen explaining why.
|
||||||
|
render(
|
||||||
|
<UrlToast
|
||||||
|
url={SIGN_IN}
|
||||||
|
onOpen={noop}
|
||||||
|
onOpenInContainer={noop}
|
||||||
|
signInDefault="host-fallback"
|
||||||
|
onDismiss={noop}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
// Which button leads does not change — only what the hint claims.
|
||||||
|
expect(actions()).toEqual(["Open", "In container"]);
|
||||||
|
expect(
|
||||||
|
document.querySelector(URL_TOAST_PRIMARY_SELECTOR),
|
||||||
|
).toHaveTextContent("Open");
|
||||||
|
const hint = screen.getByTestId("url-toast-signin-hint");
|
||||||
|
expect(hint).toHaveTextContent(/nothing is set up to reach it/i);
|
||||||
|
// And it points at the two things that would fix it, since a warning
|
||||||
|
// with no next step is only a nicer way to fail.
|
||||||
|
expect(hint).toHaveTextContent(/Auth bridge/);
|
||||||
|
expect(hint).toHaveTextContent(/install browser support/i);
|
||||||
|
expect(hint).not.toHaveTextContent(/the auth bridge is what carries the callback/i);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("defaults to the least-bad reading when the caller passes nothing", () => {
|
||||||
|
// A caller that says nothing has not told us a bridge is live, so the
|
||||||
|
// hint must not invent one. The host still leads: it is the answer more
|
||||||
|
// likely to work, and the one that reports its own failure.
|
||||||
render(
|
render(
|
||||||
<UrlToast
|
<UrlToast
|
||||||
url={SIGN_IN}
|
url={SIGN_IN}
|
||||||
@@ -233,6 +263,9 @@ describe("UrlToast", () => {
|
|||||||
/>,
|
/>,
|
||||||
);
|
);
|
||||||
expect(actions()).toEqual(["Open", "In container"]);
|
expect(actions()).toEqual(["Open", "In container"]);
|
||||||
|
expect(screen.getByTestId("url-toast-signin-hint")).toHaveTextContent(
|
||||||
|
/nothing is set up to reach it/i,
|
||||||
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("keeps the host browser available as a fallback", () => {
|
it("keeps the host browser available as a fallback", () => {
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import type { KeyboardEvent } from "react";
|
import type { KeyboardEvent } from "react";
|
||||||
import { isAnthropicSignInUrl, urlOrigin } from "../../lib/urlRelay";
|
import { isAnthropicSignInUrl, urlOrigin } from "../../lib/urlRelay";
|
||||||
|
import type { SignInOpenTarget } from "../../hooks/useSignInOpenTarget";
|
||||||
import Button from "../ui/Button";
|
import Button from "../ui/Button";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -38,17 +39,23 @@ interface Props {
|
|||||||
* the project has no browser to open it in. */
|
* the project has no browser to open it in. */
|
||||||
onOpenInContainer?: () => void;
|
onOpenInContainer?: () => void;
|
||||||
/**
|
/**
|
||||||
* Which action leads for a *sign-in* link (see the note below). Nothing else
|
* Which action leads for a *sign-in* link, and why (see the note below).
|
||||||
* in the toast moves: both buttons are offered either way, in either order.
|
* Nothing else in the toast moves: both buttons are offered in all three
|
||||||
|
* states, in one of two orders.
|
||||||
*
|
*
|
||||||
* This component does not work it out, because the answer depends on the
|
* This component does not work it out, because the answer depends on the
|
||||||
* project's auth bridge and on what is installed inside its container —
|
* project's auth bridge and on what is installed inside its container —
|
||||||
* neither of which a presentational component should be reaching for.
|
* neither of which a presentational component should be reaching for.
|
||||||
* `hooks/useSignInOpenTarget.ts` owns the rule. `"host"` is the default here
|
* `hooks/useSignInOpenTarget.ts` owns the rule.
|
||||||
* for the same reason it is the fallback there: it is the answer that is more
|
*
|
||||||
* likely to work, and the one that reports its own failure.
|
* Two of the three lead with the host button and differ only in the hint,
|
||||||
|
* which is the whole point of carrying three: `"host-bridged"` may promise
|
||||||
|
* that the auth bridge brings the callback home, `"host-fallback"` may not,
|
||||||
|
* because in that state nothing does. `"host-fallback"` is the default for
|
||||||
|
* that reason — a caller that says nothing has not told us a bridge is live,
|
||||||
|
* and the hint must not invent one.
|
||||||
*/
|
*/
|
||||||
signInDefault?: "host" | "container";
|
signInDefault?: SignInOpenTarget;
|
||||||
onDismiss: () => void;
|
onDismiss: () => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -84,6 +91,12 @@ interface Props {
|
|||||||
* passes {@link Props.signInDefault} and this only renders it: the leading
|
* passes {@link Props.signInDefault} and this only renders it: the leading
|
||||||
* button is filled and comes first, the other keeps its place beside it.
|
* button is filled and comes first, the other keeps its place beside it.
|
||||||
*
|
*
|
||||||
|
* The hint below the URL renders all *three* states, not the two orderings.
|
||||||
|
* "Neither is set up" also leads with the host, but it is not the same claim:
|
||||||
|
* there the callback has nothing carrying it, so the hint names what would fix
|
||||||
|
* that instead of describing a bridge that is off. A two-way hint keyed on
|
||||||
|
* which button leads is exactly how that false promise got shipped.
|
||||||
|
*
|
||||||
* ## Reachable without a mouse, and it does not take focus to manage it
|
* ## Reachable without a mouse, and it does not take focus to manage it
|
||||||
*
|
*
|
||||||
* This toast is the only route to completing a sign-in started in a terminal,
|
* This toast is the only route to completing a sign-in started in a terminal,
|
||||||
@@ -111,7 +124,7 @@ export default function UrlToast({
|
|||||||
label = "Long URL detected",
|
label = "Long URL detected",
|
||||||
onOpen,
|
onOpen,
|
||||||
onOpenInContainer,
|
onOpenInContainer,
|
||||||
signInDefault = "host",
|
signInDefault = "host-fallback",
|
||||||
onDismiss,
|
onDismiss,
|
||||||
}: Props) {
|
}: Props) {
|
||||||
const origin = urlOrigin(url);
|
const origin = urlOrigin(url);
|
||||||
@@ -123,6 +136,11 @@ export default function UrlToast({
|
|||||||
// container. Everything below keys off this rather than off `signIn`, so the
|
// container. Everything below keys off this rather than off `signIn`, so the
|
||||||
// two orderings differ only in which of the pair leads.
|
// two orderings differ only in which of the pair leads.
|
||||||
const containerLeads = signIn && signInDefault === "container";
|
const containerLeads = signIn && signInDefault === "container";
|
||||||
|
// The third state. Both host states put the same button first, so this is
|
||||||
|
// read by the hint alone: no bridge and no container browser means nothing is
|
||||||
|
// carrying the callback back, and saying "the auth bridge is what carries it"
|
||||||
|
// here is a promise the project cannot keep.
|
||||||
|
const hostIsLastResort = signIn && signInDefault === "host-fallback";
|
||||||
|
|
||||||
// `Button` already owns the filled/outlined variants — including the rule
|
// `Button` already owns the filled/outlined variants — including the rule
|
||||||
// that filled uses `--accent-emphasis` and never `--accent`, which is the
|
// that filled uses `--accent-emphasis` and never `--accent`, which is the
|
||||||
@@ -257,7 +275,9 @@ export default function UrlToast({
|
|||||||
>
|
>
|
||||||
{containerLeads
|
{containerLeads
|
||||||
? "Sign-in link — the callback listener is inside the container. Opening it there closes the loop; the host browser needs the auth bridge."
|
? "Sign-in link — the callback listener is inside the container. Opening it there closes the loop; the host browser needs the auth bridge."
|
||||||
: "Sign-in link — the callback listener is inside the container. The auth bridge is what carries the callback back to it from your own browser."}
|
: hostIsLastResort
|
||||||
|
? "Sign-in link — the callback listener is inside the container and nothing is set up to reach it. Turn on Auth bridge in the project’s Config tab, or install browser support to sign in inside the container."
|
||||||
|
: "Sign-in link — the callback listener is inside the container. The auth bridge is what carries the callback back to it from your own browser."}
|
||||||
</div>
|
</div>
|
||||||
)}
|
)}
|
||||||
</div>
|
</div>
|
||||||
|
|||||||
@@ -14,8 +14,28 @@ import type {
|
|||||||
/** Emitted by `auth_bridge/mod.rs` whenever the port or conflict set changes. */
|
/** Emitted by `auth_bridge/mod.rs` whenever the port or conflict set changes. */
|
||||||
const AUTH_BRIDGE_EVENT = "auth-bridge-changed";
|
const AUTH_BRIDGE_EVENT = "auth-bridge-changed";
|
||||||
|
|
||||||
/** Which of the URL toast's two buttons should lead for a sign-in link. */
|
/**
|
||||||
export type SignInOpenTarget = "host" | "container";
|
* Which of the URL toast's two buttons should lead for a sign-in link — and,
|
||||||
|
* for the host, *why*.
|
||||||
|
*
|
||||||
|
* Three states rather than two because "host" covers two worlds that are not
|
||||||
|
* the same promise to the user:
|
||||||
|
*
|
||||||
|
* - `host-bridged` — the auth bridge is live, so a sign-in completed in the
|
||||||
|
* user's own browser has its callback carried back to the listener inside
|
||||||
|
* the container. The host is genuinely the better answer here.
|
||||||
|
* - `container` — no bridge, but the container has a browser to open, which
|
||||||
|
* closes the loop locally with nothing crossing to the host.
|
||||||
|
* - `host-fallback` — neither. The host is the *least bad* of two answers
|
||||||
|
* that can both fail, and the toast has to say so: a hint claiming the
|
||||||
|
* bridge will carry the callback is a false promise in this state, and the
|
||||||
|
* user's `claude login` hangs to its timeout with nothing explaining why.
|
||||||
|
*
|
||||||
|
* Only `container` changes which button leads; the split between the two host
|
||||||
|
* states exists so the toast's hint can tell the truth. Keep it that way — the
|
||||||
|
* consumer that folds them back together is the bug this replaced.
|
||||||
|
*/
|
||||||
|
export type SignInOpenTarget = "host-bridged" | "container" | "host-fallback";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Whether the auth bridge can be relied on to catch a callback for this
|
* Whether the auth bridge can be relied on to catch a callback for this
|
||||||
@@ -42,16 +62,19 @@ export function authBridgeIsLive(status: AuthBridgeStatus | null): boolean {
|
|||||||
/**
|
/**
|
||||||
* The rule, as a pure function of the two things it depends on.
|
* The rule, as a pure function of the two things it depends on.
|
||||||
*
|
*
|
||||||
* Both fallbacks land on the host, for different reasons:
|
* Both host answers land on the same button, for different reasons — and they
|
||||||
|
* are deliberately *not* the same value:
|
||||||
*
|
*
|
||||||
* - With the bridge live, the host browser is strictly better — it is the
|
* - With the bridge live (`host-bridged`), the host browser is strictly
|
||||||
* user's own signed-in profile, and the callback still reaches the container.
|
* better — it is the user's own signed-in profile, and the callback still
|
||||||
* - With neither available, the host is the *more likely to work* of two
|
* reaches the container.
|
||||||
* imperfect answers, and it is the one that reports its own failure (see
|
* - With neither available (`host-fallback`), the host is the *more likely to
|
||||||
* `handleOpenUrl` in `TerminalView`). The container-side target is
|
* work* of two imperfect answers, and it is the one that reports its own
|
||||||
* Playwright's dashboard pane, and Playwright's browsers are not baked into
|
* failure (see `handleOpenUrl` in `TerminalView`). The container-side target
|
||||||
* the image, so on a fresh project pointing there fails on every platform
|
* is Playwright's dashboard pane, and Playwright's browsers are not baked
|
||||||
* after a several-second wait.
|
* into the image, so on a fresh project pointing there fails on every
|
||||||
|
* platform after a several-second wait. Nothing carries the callback back in
|
||||||
|
* this state, so the toast says so rather than promising the bridge.
|
||||||
*
|
*
|
||||||
* Whichever way it goes, both buttons stay in the toast. This chooses which one
|
* Whichever way it goes, both buttons stay in the toast. This chooses which one
|
||||||
* leads, never which ones exist.
|
* leads, never which ones exist.
|
||||||
@@ -60,9 +83,9 @@ export function chooseSignInTarget(
|
|||||||
bridge: AuthBridgeStatus | null,
|
bridge: AuthBridgeStatus | null,
|
||||||
detection: PlaywrightDetection | null,
|
detection: PlaywrightDetection | null,
|
||||||
): SignInOpenTarget {
|
): SignInOpenTarget {
|
||||||
if (authBridgeIsLive(bridge)) return "host";
|
if (authBridgeIsLive(bridge)) return "host-bridged";
|
||||||
if (canOpenPageInContainerBrowser(detection)) return "container";
|
if (canOpenPageInContainerBrowser(detection)) return "container";
|
||||||
return "host";
|
return "host-fallback";
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -113,11 +136,16 @@ export function resetBrowserSupportCache(): void {
|
|||||||
* bridge now on by default, is the ordinary case.
|
* bridge now on by default, is the ordinary case.
|
||||||
*/
|
*/
|
||||||
export function useSignInOpenTarget(projectId: string | undefined): SignInOpenTarget {
|
export function useSignInOpenTarget(projectId: string | undefined): SignInOpenTarget {
|
||||||
const [target, setTarget] = useState<SignInOpenTarget>("host");
|
// `host-fallback` is the honest starting point, not `host-bridged`: before
|
||||||
|
// the status call answers, nothing is known to be carrying the callback, and
|
||||||
|
// the hint that claims one is the failure this three-state answer exists to
|
||||||
|
// prevent. Over-warning for the moment before the answer arrives costs a line
|
||||||
|
// of hedged text; under-warning costs a login that hangs to its timeout.
|
||||||
|
const [target, setTarget] = useState<SignInOpenTarget>("host-fallback");
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
if (!projectId) {
|
if (!projectId) {
|
||||||
setTarget("host");
|
setTarget("host-fallback");
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -145,8 +173,10 @@ export function useSignInOpenTarget(projectId: string | undefined): SignInOpenTa
|
|||||||
.then((s) => {
|
.then((s) => {
|
||||||
if (!cancelled) consider(s);
|
if (!cancelled) consider(s);
|
||||||
})
|
})
|
||||||
// Nothing to say to the user here: this only picks which button is
|
// Nothing to say to the user here: an unanswered status call is fed
|
||||||
// filled in, and the fallback is the one that reports its own failures.
|
// through as a bridge that is off, which lands on `container` or
|
||||||
|
// `host-fallback` — and `host-fallback`'s hint is the one that tells the
|
||||||
|
// user the callback has nothing carrying it.
|
||||||
.catch(() => {
|
.catch(() => {
|
||||||
if (!cancelled) consider({ enabled: false, active_ports: [], conflicts: [] });
|
if (!cancelled) consider({ enabled: false, active_ports: [], conflicts: [] });
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user