Skip to content

refactor: extract validation and socat cleanup from stop.rs handle#126

Merged
jamiesunderland merged 1 commit intocoast-guard:mainfrom
mukeshblackhat:refactor/stop-extract-validation-122
Mar 23, 2026
Merged

refactor: extract validation and socat cleanup from stop.rs handle#126
jamiesunderland merged 1 commit intocoast-guard:mainfrom
mukeshblackhat:refactor/stop-extract-validation-122

Conversation

@mukeshblackhat
Copy link
Contributor

Summary

  • Extracted validate_stoppable — pure function that checks if an instance status allows stopping
  • Extracted kill_instance_socat_processes — kills socat PIDs and clears them in the DB
  • Updated handle to call both helpers instead of inline logic
  • Added 11 unit tests (8 for validation, 3 for socat cleanup)
  • #[allow(clippy::cognitive_complexity, clippy::too_many_lines)] kept on handle — still triggers both lints after extraction (follow-up ticket)

What was there before

handle was ~220 lines with status validation (lines 64-88) and socat cleanup (lines 200-216) inlined. The status checks were a chain of if statements comparing against individual InstanceStatus variants.

What changed

Single file: coast-daemon/src/handlers/stop.rs

Extracted functions:

Function Type What it does
validate_stoppable(status, name) Pure, sync Returns Ok for Running/CheckedOut/Idle, Err for Stopped and transitional states
kill_instance_socat_processes(db, project, name) Sync Iterates port allocations, kills socat PIDs, clears them in DB

Deviations from ticket suggestions

Ticket suggested What we did Why
Idle grouped with Enqueued (not stoppable) Idle grouped with Running/CheckedOut (stoppable) Original code never rejected Idle — it fell through to the stoppable path. coast archive depends on this: handle_archive calls stop::handle for Idle instances. Rejecting Idle broke test_handle_archive_stops_only_running_checked_out_and_idle_instances (expected 3 stopped, got 2). Fixed by preserving the original behavior.
kill_instance_socat_processes as async fn Made it fn (sync) All operations inside are synchronous — DB reads via StateDb and process signals via kill_socat. StateDb is not Send, so marking the function async causes compilation errors. The ticket's suggestion was reasonable but doesn't work with the current type constraints.

Test plan

Run new tests

# 17 tests pass (6 existing + 11 new)
cargo test -p coast-daemon -- stop::tests

Verify archive tests still pass

# This test caught the Idle behavioral regression — must pass
cargo test -p coast-daemon -- archive

Run lint and full tests

make lint    # zero warnings
make test    # all pass

Verify behavior is unchanged

# coast stop should work for Running, CheckedOut, and Idle instances
# coast stop should error for Stopped and transitional states
# coast archive should stop all Running + CheckedOut + Idle instances

Closes #122

Extract two helpers from the stop handler to reduce complexity and add
test coverage:

- validate_stoppable: pure status check for Running, CheckedOut, Idle
  (ok) vs Stopped, transitional states (error). Enqueued is handled
  separately by the caller.
- kill_instance_socat_processes: iterates port allocations, kills socat
  PIDs, and clears them in the DB.

Adds 11 unit tests (8 for validate_stoppable, 3 for socat cleanup).
The #[allow] annotations on handle remain as it still triggers both
lints after extraction.

Closes coast-guard#122
Copy link
Contributor

@jamiesunderland jamiesunderland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks @mukeshblackhat

@jamiesunderland jamiesunderland merged commit 4880104 into coast-guard:main Mar 23, 2026
2 checks passed
mukeshblackhat added a commit to mukeshblackhat/coasts that referenced this pull request Mar 24, 2026
Extract status validation into a pure function with an exhaustive match
on all InstanceStatus variants, following the same pattern as
validate_stoppable in stop.rs (PR coast-guard#126).

Startable: Stopped, Idle, Enqueued, Unassigning (preserving original
fall-through behavior). Rejected: Running/CheckedOut (already running),
Provisioning/Assigning/Starting/Stopping (transitional).

Adds 10 unit tests covering every InstanceStatus variant. The #[allow]
annotations on handle remain as it still triggers both lints.

Closes coast-guard#133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extract validation and cleanup logic from stop.rs:handle to reduce complexity and add test coverage

2 participants