From 1e9b71c8ab0f8b5a876d7a279164d07effe647b8 Mon Sep 17 00:00:00 2001 From: Evan Jacobs Date: Fri, 7 Aug 2026 23:31:28 -0400 Subject: [PATCH 1/9] fix(lock): stop leaking options into the command and name a contended holder The parse defect was structural. `.captureForPassthrough` ends the option loop at the first positional value, so the resource itself stopped option parsing and `--timeout 300 -- cmd` all landed in the command vector. Declaration order can never fix that, and the comment claiming it could was wrong, as was the one blaming @Flag inversion for the same symptom. `.postTerminator` lifts everything after `--` before positionals are filled, which also deletes the hand-scrape that rescued only `--no-pause` and makes --help finally show the terminator the contract documents. The acquire wait was silent, so a second run looked hung and the reflex was to kill whichever run held the lock. A new read-only lock.status answers who holds it, and the CLI names the pid, the hold's age and what it paused or left running, then repeats while waiting. The loop became a schedule so --acquire-timeout 0 makes one attempt; the old `while Date() < deadline` never entered its body at budget 0 and failed naming no holder. Every notice moved to stderr, which lets the smoke gate parse lock's stdout as plain JSON. New CLI test target: the parse behavior has a contract and no other way to exercise it. Its tests fail on the old strategy with the reported argv. --- .changeset/lock-parsing-and-contention.md | 7 + BACKLOG.md | 1 - Package.swift | 11 ++ .../Control/ControlServer.swift | 58 ++++-- Sources/DevCtlKit/Protocol/Wire.swift | 38 +++- Sources/devctl/CLI.swift | 167 +++++++++++++++--- Tests/DevCtlCLITests/LockParsingTests.swift | 76 ++++++++ Tests/DevCtlCLITests/LockWaitTests.swift | 89 ++++++++++ .../ResourceLockTests.swift | 90 ++++++++++ docs/cli-contract.md | 2 +- scripts/smoke.sh | 39 +++- 11 files changed, 525 insertions(+), 53 deletions(-) create mode 100644 .changeset/lock-parsing-and-contention.md create mode 100644 Tests/DevCtlCLITests/LockParsingTests.swift create mode 100644 Tests/DevCtlCLITests/LockWaitTests.swift diff --git a/.changeset/lock-parsing-and-contention.md b/.changeset/lock-parsing-and-contention.md new file mode 100644 index 0000000..19e61c6 --- /dev/null +++ b/.changeset/lock-parsing-and-contention.md @@ -0,0 +1,7 @@ +--- +"devctl": minor +--- + +`devctl lock` no longer passes its own options to the guarded command. `devctl lock d1 --timeout 300 -- cmd` ran `env --timeout 300 -- cmd` and died with `env: illegal option -- t`, because the resource name ended option parsing and everything after it was captured as the command. The command is now taken from after `--` verbatim, so a nested `--`, a dash option, and an empty string all survive, while a missing terminator or an unknown option is rejected instead of quietly passed through. + +A contended `devctl lock` says who holds the resource instead of blocking silently for up to five minutes. It names the holder's pid, how long that run has been going, and which servers it paused or left running, then repeats a still-waiting line while it waits. Silence there reads as a hung gate, and the reflex it invites is killing the run that holds the lock, which is the one making progress. `--acquire-timeout 0` now makes exactly one attempt and fails immediately, which it could not do before: the wait loop never ran its body at a zero budget and failed with a message naming no holder. All of `lock`'s own output moved to stderr, so stdout carries only the guarded command's. diff --git a/BACKLOG.md b/BACKLOG.md index fba09eb..fb257d7 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -14,7 +14,6 @@ Open work only; entries are removed by the change that resolves them. - `IntegrationTests` is a single `placeholder()` while `docs/design.md` promises a real end-to-end suite there (port-conflict from a second project, concurrent double-ensure). Those now live in `scripts/smoke.sh` and unit suites; either build the integration target out or retire the promise in the design doc. - A restarting daemon is indistinguishable from a dead one for the length of `recoverAtStartup`, which is seconds when there is real state to restore. The listener only reaches `.ready` after restore, and the socket is unlinked in `ControlServer.init`, so clients get `daemon-unreachable` with ENOENT and nothing that says "starting". Moving the unlink later only changes the errno, since the daemon is unreachable either way: the fix is to answer during restore (bind early and reply "starting" to everything, or write a state file clients can read) so `devctl daemon status` can say restoring rather than down. Wanted because install/restart bounce servers, and an agent polling through that window sees a daemon that looks gone. - Split `CLI.swift`: every command struct lives in that one file, which is past the size where splitting is worth asking about. A structure decision, not drift; the codebase map describes the current layout. -- `lock` mis-parses its own options after the resource: `devctl lock d1 --timeout 300 -- ` passed `--timeout` through to the command, which failed with `env: illegal option -- t`. The same call without `--timeout` worked. Either the option has to precede the resource (undocumented, and `--help` lists it after ``) or the parser is leaking pre-`--` options into the command vector. A stray flag reaching the command is a silent behavior change in the worst case, not just a usage error. - Field-level config editing in the dashboard would preserve `devservers.json` formatting instead of normalizing writes. `devctl config init` writes indented JSON with sorted keys, so a recovered file and a dashboard-saved file now agree on shape, but a hand-authored one with its own ordering or comments still normalizes on save. ## `lock --no-pause` is not enough when the command deletes the locked state diff --git a/Package.swift b/Package.swift index 6a0b773..97e18ba 100644 --- a/Package.swift +++ b/Package.swift @@ -78,6 +78,17 @@ let package = Package( name: "DevCtlDaemonCoreTests", dependencies: ["DevCtlDaemonCore", "DevCtlKit"] ), + /** The CLI's argument parsing is behavior with a contract (docs/cli-contract.md) + and no other way to exercise it: a parse defect there silently changes + what a guarded command receives. */ + .testTarget( + name: "DevCtlCLITests", + dependencies: [ + "DevCtlKit", + "devctl", + .product(name: "ArgumentParser", package: "swift-argument-parser"), + ] + ), .testTarget( name: "IntegrationTests", dependencies: ["DevCtlKit"] diff --git a/Sources/DevCtlDaemonCore/Control/ControlServer.swift b/Sources/DevCtlDaemonCore/Control/ControlServer.swift index a405743..765f804 100644 --- a/Sources/DevCtlDaemonCore/Control/ControlServer.swift +++ b/Sources/DevCtlDaemonCore/Control/ControlServer.swift @@ -229,6 +229,12 @@ public actor Router { params.project = canonicalProjectPath(params.project) let result = try await acquireLock(params) return try respond(id: head.id, result: result) + case .lockStatus: + let request = try decoder.decode(WireRequest.self, from: line) + let params = LockStatusParams( + project: canonicalProjectPath(request.params.project), + resource: request.params.resource) + return try respond(id: head.id, result: await lockStatus(params)) case .lockRelease: let request = try decoder.decode(WireRequest.self, from: line) var params = request.params @@ -1066,32 +1072,48 @@ public actor Router { if let existing = resourceLocks[key], existing.pid == params.holderPid { return LockResult(paused: existing.paused) } + var live: [String] = [] var paused: [String] = [] let shouldPause = params.pause ?? true let merged = try? await mergedSpecs(project: params.project) - if shouldPause { - for spec in merged?.specs ?? [] where (spec.locks ?? []).contains(params.resource) { - let supervisor = await supervisor(project: params.project, spec: spec) - let status = await supervisor.status() - switch status.phase { - case .running, .starting, .unhealthy, .stopping: - /** Non-retiring stop: boot intent survives so a daemon crash - mid-hold can still bring the server back if the holder is gone. */ - _ = await supervisor.stop(deliberate: false) - paused.append(spec.name) - DevCtlLog.daemon.info( - "lock \(params.resource) paused \(spec.name)@\(params.project)") - case .stopped, .crashed, .failed: - break + for spec in merged?.specs ?? [] where (spec.locks ?? []).contains(params.resource) { + let supervisor = await supervisor(project: params.project, spec: spec) + let status = await supervisor.status() + switch status.phase { + case .running, .starting, .unhealthy, .stopping: + guard shouldPause else { + /** Sound for the whole hold: lockGate refuses to start a + declarer while a live holder owns the resource, so this + set can only shrink. */ + live.append(spec.name) + continue } + /** Non-retiring stop: boot intent survives so a daemon crash + mid-hold can still bring the server back if the holder is gone. */ + _ = await supervisor.stop(deliberate: false) + paused.append(spec.name) + DevCtlLog.daemon.info( + "lock \(params.resource) paused \(spec.name)@\(params.project)") + case .stopped, .crashed, .failed: + break } - paused.sort() } + live.sort() + paused.sort() resourceLocks[key] = LockHolder( - paused: paused, pid: params.holderPid, - resumeTimeoutSeconds: params.resumeTimeoutSeconds, since: Date()) + live: live.isEmpty ? nil : live, pause: shouldPause, paused: paused, + pid: params.holderPid, resumeTimeoutSeconds: params.resumeTimeoutSeconds, since: Date()) persistLocks() - return LockResult(paused: paused) + return LockResult(live: live.isEmpty ? nil : live, paused: paused) + } + + /** Who holds a resource right now, if anyone. A dead holder is released + first, so a stale row reads as no holder rather than as a phantom the + caller then waits on. */ + private func lockStatus(_ params: LockStatusParams) async -> LockStatusResult { + let key = Self.lockKey(project: params.project, resource: params.resource) + await releaseOrphanedLock(key: key) + return LockStatusResult(holder: resourceLocks[key]) } /** Release: only the matching holder clears the lock; then ensure everyone diff --git a/Sources/DevCtlKit/Protocol/Wire.swift b/Sources/DevCtlKit/Protocol/Wire.swift index 2a64328..5889ffc 100644 --- a/Sources/DevCtlKit/Protocol/Wire.swift +++ b/Sources/DevCtlKit/Protocol/Wire.swift @@ -176,6 +176,7 @@ public enum WireMethod: String, Sendable { case groupUp = "group.up" case lockAcquire = "lock.acquire" case lockRelease = "lock.release" + case lockStatus = "lock.status" case logsMark = "logs.mark" case logsQuery = "logs.query" case projectCheck = "project.check" @@ -331,14 +332,23 @@ public struct LockParams: Codable, Equatable, Sendable { /** In-memory and on-disk lock record. `paused` is who the daemon stopped for this hold so a crash mid-lock can resume them when the holder is gone. */ public struct LockHolder: Codable, Equatable, Sendable { + /** Declaring servers this hold left running (`--no-pause`). Absent under the + default paused mode, and on files written before this field existed. */ + public var live: [String]? + /** Whether this hold paused declarers. An empty `paused` is ambiguous on its + own: nothing was running, or nothing was asked to stop. */ + public var pause: Bool? public var paused: [String] public var pid: Int public var resumeTimeoutSeconds: Double? public var since: Date public init( - paused: [String] = [], pid: Int, resumeTimeoutSeconds: Double? = nil, since: Date + live: [String]? = nil, pause: Bool? = nil, paused: [String] = [], pid: Int, + resumeTimeoutSeconds: Double? = nil, since: Date ) { + self.live = live + self.pause = pause self.paused = paused self.pid = pid self.resumeTimeoutSeconds = resumeTimeoutSeconds @@ -346,11 +356,35 @@ public struct LockHolder: Codable, Equatable, Sendable { } } +public struct LockStatusParams: Codable, Equatable, Sendable { + public var project: String + public var resource: String + + public init(project: String, resource: String) { + self.project = project + self.resource = resource + } +} + +/** Who, if anyone, holds a resource right now. Read-only: a contended acquire + asks once so the waiting run can name the holder instead of looking hung, + which is what stops someone killing the run that is making progress. */ +public struct LockStatusResult: Codable, Equatable, Sendable { + public var holder: LockHolder? + + public init(holder: LockHolder? = nil) { + self.holder = holder + } +} + /** Result of lock.acquire / lock.release: which servers were paused or resumed. */ public struct LockResult: Codable, Equatable, Sendable { + /** Declaring servers this hold left running, under `--no-pause`. */ + public var live: [String]? public var paused: [String] - public init(paused: [String] = []) { + public init(live: [String]? = nil, paused: [String] = []) { + self.live = live self.paused = paused } } diff --git a/Sources/devctl/CLI.swift b/Sources/devctl/CLI.swift index ec97bd6..fd7975f 100644 --- a/Sources/devctl/CLI.swift +++ b/Sources/devctl/CLI.swift @@ -1560,6 +1560,73 @@ struct Switch: AsyncParsableCommand { } } +/** The acquire wait as a decision rather than a loop condition, so a zero budget + still makes one attempt. `while Date() < deadline` never entered its body at + budget 0, which produced a generic failure naming no holder. */ +struct LockAcquireSchedule: Equatable, Sendable { + static let announceIntervalSeconds: Double = 15 + static let retryIntervalSeconds: Double = 1 + var budgetSeconds: Double + + func shouldRetry(afterElapsed elapsed: Double) -> Bool { + elapsed < budgetSeconds + } + + func shouldAnnounceStillWaiting(atElapsed elapsed: Double, lastAnnouncedElapsed: Double?) + -> Bool + { + guard let last = lastAnnouncedElapsed else { return false } + return elapsed - last >= Self.announceIntervalSeconds + } +} + +/** Everything a contended acquire says, pure so the exact wording is asserted. + Silence here is what made a waiting run look hung, and the reflex that + invites is killing whichever run holds the lock, which is the one making + progress. Every line goes to stderr: stdout belongs to the guarded command. */ +enum LockNotice { + static func contended( + budgetSeconds: Double, holder: LockHolder, now: Date, resource: String + ) -> String { + let age = DurationText.brief(seconds: now.timeIntervalSince(holder.since)) + var lines = [ + "devctl lock: '\(resource)' is held by pid \(holder.pid), running for \(age)\(pauseClause(holder))." + ] + lines.append( + "devctl lock: waiting up to \(DurationText.brief(seconds: budgetSeconds)) for that run to finish. It is the one making progress, so check it with `ps -p \(holder.pid)` before killing anything." + ) + return lines.joined(separator: "\n") + } + + static func stillWaiting( + elapsedSeconds: Double, holder: LockHolder, remainingSeconds: Double, resource: String + ) -> String { + "devctl lock: still waiting on '\(resource)' (pid \(holder.pid)), \(DurationText.brief(seconds: elapsedSeconds)) elapsed, \(DurationText.brief(seconds: remainingSeconds)) left." + } + + private static func pauseClause(_ holder: LockHolder) -> String { + if holder.pause == false { + guard let live = holder.live, !live.isEmpty else { + return " (it left declaring servers running, --no-pause)" + } + return " (it left \(live.joined(separator: ", ")) running, --no-pause)" + } + guard !holder.paused.isEmpty else { return " (nothing was running to pause)" } + return " (it paused \(holder.paused.joined(separator: ", ")))" + } +} + +/** Compact human durations. Nothing else in the CLI formats one. */ +enum DurationText { + static func brief(seconds: Double) -> String { + let total = Int(seconds.rounded()) + guard total >= 60 else { return "\(max(total, 0))s" } + let minutes = total / 60 + guard minutes >= 60 else { return String(format: "%dm %02ds", minutes, total % 60) } + return String(format: "%dh %02dm", minutes / 60, minutes % 60) + } +} + /** Runs a command while holding a named resource exclusively. By default the daemon pauses managed servers that declare the resource; `--no-pause` takes the mutex without stopping them (for harnesses that reuse the live server). */ @@ -1569,50 +1636,64 @@ struct Lock: AsyncParsableCommand { @OptionGroup var global: GlobalOptions - /** Positional order is load-bearing: the resource comes first, everything - after -- is the command, so declaration order deliberately breaks the - alphabet. Options and flags must appear before the passthrough argv so - `--no-pause` is not captured into the command. */ + /** Declaration order is load-bearing for the help synopsis only: the + repeating positional has to come last, and the options render in + declaration order, which is what makes `--help` match the contract. It has + no bearing on parsing, since `.postTerminator` lifts everything after `--` + before any positional is filled. */ @Argument(help: "Resource name (matches servers' `locks` in devservers.json).") var resource: String @Option(help: "Seconds to wait for the resource if another holder has it.") var acquireTimeout: Double = 300 - /** Explicit long name: `@Flag(inversion: .prefixedNo)` on a default-true - `pause` was still pausing under `--no-pause` in the smoke gate (daemon - unit tests with `pause: false` were fine), so the wire bit is driven by - this opt-out flag instead. */ + /** Spelled as an explicit opt-out rather than `@Flag(inversion: .prefixedNo)` + on a default-true `pause`: the contract documents this spelling, and the + inverted form would also mint a `--pause` that does nothing. The symptom + that first blamed inversion was the passthrough parse below swallowing the + flag into the command. */ @Flag(name: .customLong("no-pause"), help: "Hold the mutex without stopping servers that declare the resource.") var noPause = false @Option(help: "Per-server seconds to wait for health when servers return.") var timeout: Double = 120 - @Argument(parsing: .captureForPassthrough, help: "Command to run while holding the resource.") - var command: [String] + /** `.postTerminator`, not `.captureForPassthrough`: the latter ends option + parsing at the first positional value, so the resource itself stopped it + and `--timeout 300` joined the guarded command, which then ran as + `env --timeout 300 -- cmd`. This strategy lifts everything after `--` + verbatim (a nested `--`, a dash option, an empty string all survive) and + leaves the options to parse normally. The default makes a missing command + reach the typed usage error below rather than the parser's own printer. */ + @Argument(parsing: .postTerminator, help: "Command to run while holding the resource; everything after `--`.") + var command: [String] = [] + + /** Pure so the exact message is asserted without spawning the CLI. */ + static func usageError(command: [String], resource: String) -> WireError? { + guard command.isEmpty else { return nil } + return WireError( + code: .usage, + hint: "devctl lock \(resource) -- ", + message: "devctl lock needs a command after `--`; its own options go before it (devctl lock \(resource) [--no-pause] [--acquire-timeout ] [--timeout ] -- )") + } func run() async throws { - var noPause = noPause - var command = command - if !noPause, let flagIndex = command.firstIndex(of: "--no-pause") { - noPause = true - command.remove(at: flagIndex) - } - guard !command.isEmpty else { - CLIRunner.fail( - WireError(code: .usage, message: "usage: devctl lock [--no-pause] -- "), - json: global.json) + let command = command + let noPause = noPause + if let usage = Self.usageError(command: command, resource: resource) { + CLIRunner.fail(usage, json: global.json) } let project = global.resolvedProject() let holderPid = Int(getpid()) let client = CLIRunner.client() /** Acquire with patience: another harness may hold it. The daemon owns pause/resume of declaring servers; this CLI just runs the command. */ - let deadline = Date().addingTimeInterval(acquireTimeout) + let schedule = LockAcquireSchedule(budgetSeconds: acquireTimeout) + let started = Date() var acquired: LockResult? + var announcedAt: Double? var lastError: WireError? - while Date() < deadline { + repeat { do { acquired = try await client.request( .lockAcquire, @@ -1623,16 +1704,46 @@ struct Lock: AsyncParsableCommand { break } catch let error as WireError where error.code == .resourceLocked { lastError = error - try? await Task.sleep(for: .seconds(1)) + let elapsed = Date().timeIntervalSince(started) + /** Ask who holds it once, then repeat on an interval so a long + wait stays visibly bounded rather than silent. An older daemon + without lock.status degrades to the bounded-wait line. */ + let holder = try? await client.request( + .lockStatus, + params: LockStatusParams(project: project, resource: resource), + expecting: LockStatusResult.self + ).holder + if let holder { + if announcedAt == nil { + Self.note( + LockNotice.contended( + budgetSeconds: acquireTimeout, holder: holder, now: Date(), + resource: resource)) + announcedAt = elapsed + } else if schedule.shouldAnnounceStillWaiting( + atElapsed: elapsed, lastAnnouncedElapsed: announcedAt) + { + Self.note( + LockNotice.stillWaiting( + elapsedSeconds: elapsed, holder: holder, + remainingSeconds: max(acquireTimeout - elapsed, 0), + resource: resource)) + announcedAt = elapsed + } + } + guard schedule.shouldRetry(afterElapsed: elapsed) else { break } + try? await Task.sleep(for: .seconds(LockAcquireSchedule.retryIntervalSeconds)) } - } + } while schedule.shouldRetry(afterElapsed: Date().timeIntervalSince(started)) guard let acquired else { CLIRunner.fail( lastError ?? WireError(code: .resourceLocked, message: "could not acquire '\(resource)'"), json: global.json) } + /** Progress chatter is stderr: stdout belongs to the guarded command, and + --json governs stdout schemas. */ for name in acquired.paused { - print("paused \(name) (holds \(resource))") + Self.note("devctl lock: paused \(name) (holds \(resource))") } /** Run the guarded command with inherited stdio. */ let process = Process() @@ -1655,8 +1766,12 @@ struct Lock: AsyncParsableCommand { resumeTimeoutSeconds: timeout), expecting: LockResult.self)) ?? LockResult() for name in released.paused { - print("resuming \(name)…") + Self.note("devctl lock: resuming \(name)…") } Foundation.exit(commandStatus) } + + static func note(_ text: String) { + FileHandle.standardError.write(Data((text + "\n").utf8)) + } } diff --git a/Tests/DevCtlCLITests/LockParsingTests.swift b/Tests/DevCtlCLITests/LockParsingTests.swift new file mode 100644 index 0000000..1850418 --- /dev/null +++ b/Tests/DevCtlCLITests/LockParsingTests.swift @@ -0,0 +1,76 @@ +import ArgumentParser +import DevCtlKit +import Foundation +import Testing + +@testable import devctl + +/** `devctl lock --timeout 300 -- cmd` ran `env --timeout 300 -- cmd` + and died with `env: illegal option -- t`. In swift-argument-parser, + `.captureForPassthrough` ends the option loop at the first positional value, + so the resource itself stopped option parsing and everything after it joined + the guarded command. Declaration order cannot fix that; `.postTerminator` + can, because it lifts everything after `--` before any positional is filled. */ +@Suite struct LockParsingTests { + @Test func optionsBeforeTheTerminatorReachTheirProperties() throws { + let lock = try Lock.parse([ + "d1", "--acquire-timeout", "5", "--timeout", "300", "--", "somecmd", + ]) + #expect(lock.resource == "d1") + #expect(lock.acquireTimeout == 5) + #expect(lock.timeout == 300) + #expect(lock.command == ["somecmd"]) + } + + @Test func noPauseIsAFlagRatherThanAScrapedToken() throws { + let after = try Lock.parse(["d1", "--no-pause", "--", "cmd"]) + #expect(after.noPause) + #expect(after.command == ["cmd"]) + let before = try Lock.parse(["--no-pause", "d1", "--", "cmd"]) + #expect(before.noPause) + #expect(before.command == ["cmd"]) + } + + /** Everything after the terminator is a value, so a nested `--`, a dash + option, and an empty string all survive untouched. */ + @Test func theGuardedCommandIsCapturedVerbatim() throws { + let lock = try Lock.parse([ + "d1", "--", "git", "--", "path", "-x", "--json", "", + ]) + #expect(lock.command == ["git", "--", "path", "-x", "--json", ""]) + } + + @Test func aMissingTerminatorIsRejected() { + #expect(throws: (any Error).self) { + _ = try Lock.parse(["d1", "somecmd"]) + } + } + + @Test func anUnknownOptionIsRejected() { + #expect(throws: (any Error).self) { + _ = try Lock.parse(["d1", "--typo", "--", "cmd"]) + } + } + + /** The contract documents the terminator, and the help had never shown it. */ + @Test func helpShowsTheTerminatorAndEveryOption() { + let help = Lock.helpMessage() + #expect(help.contains("--")) + #expect(help.contains("--acquire-timeout")) + #expect(help.contains("--no-pause")) + #expect(help.contains("--timeout")) + } + + @Test func anEmptyCommandProducesTheTypedUsageError() throws { + let lock = try Lock.parse(["d1", "--"]) + #expect(lock.command.isEmpty) + let error = try #require(Lock.usageError(command: lock.command, resource: lock.resource)) + #expect(error.code == .usage) + #expect(error.hint == "devctl lock d1 -- ") + #expect(error.message.contains("needs a command after `--`")) + } + + @Test func aPresentCommandProducesNoUsageError() { + #expect(Lock.usageError(command: ["true"], resource: "d1") == nil) + } +} diff --git a/Tests/DevCtlCLITests/LockWaitTests.swift b/Tests/DevCtlCLITests/LockWaitTests.swift new file mode 100644 index 0000000..bf1f4fa --- /dev/null +++ b/Tests/DevCtlCLITests/LockWaitTests.swift @@ -0,0 +1,89 @@ +import DevCtlKit +import Foundation +import Testing + +@testable import devctl + +/** A contended acquire used to block for up to five minutes with nothing on + stdout, which reads as a hung gate. The reflex that invites is killing + whichever run holds the lock, and that run is the one making progress. */ +@Suite struct LockWaitTests { + private let since = Date(timeIntervalSince1970: 1_752_868_000) + private var now: Date { since.addingTimeInterval(125) } + + @Test func contendedNoticeNamesTheHolderItsAgeAndWhatItPaused() { + let holder = LockHolder( + pause: true, paused: ["db", "web"], pid: 4242, since: since) + let text = LockNotice.contended( + budgetSeconds: 300, holder: holder, now: now, resource: "d1") + #expect( + text == """ + devctl lock: 'd1' is held by pid 4242, running for 2m 05s (it paused db, web). + devctl lock: waiting up to 5m 00s for that run to finish. It is the one making progress, so check it with `ps -p 4242` before killing anything. + """) + } + + @Test func contendedNoticeSaysWhichServersAHolderLeftRunning() { + let holder = LockHolder(live: ["db"], pause: false, paused: [], pid: 77, since: since) + let text = LockNotice.contended( + budgetSeconds: 300, holder: holder, now: now, resource: "d1") + #expect(text.contains("(it left db running, --no-pause)")) + } + + /** An empty paused set is ambiguous without the pause bit, so it gets its + own sentence rather than reading as "it paused nothing you care about". */ + @Test func contendedNoticeDistinguishesNothingRunningFromNoPause() { + let holder = LockHolder(pause: true, paused: [], pid: 9, since: since) + let text = LockNotice.contended( + budgetSeconds: 60, holder: holder, now: now, resource: "d1") + #expect(text.contains("(nothing was running to pause)")) + } + + /** A holder written by an older daemon has no pause bit at all. */ + @Test func contendedNoticeReadsSensiblyForAPreFeatureHolder() { + let holder = LockHolder(paused: ["db"], pid: 5, since: since) + let text = LockNotice.contended( + budgetSeconds: 300, holder: holder, now: now, resource: "d1") + #expect(text.contains("(it paused db)")) + } + + @Test func stillWaitingCountsElapsedAndRemaining() { + let holder = LockHolder(pause: true, paused: [], pid: 4242, since: since) + let text = LockNotice.stillWaiting( + elapsedSeconds: 45, holder: holder, remainingSeconds: 255, resource: "d1") + #expect( + text + == "devctl lock: still waiting on 'd1' (pid 4242), 45s elapsed, 4m 15s left.") + } + + /** Budget 0 is the fail-fast form a script wants. The old `while` condition + never entered its body there, so it failed with no holder named. */ + @Test func zeroBudgetMakesExactlyOneAttempt() { + let schedule = LockAcquireSchedule(budgetSeconds: 0) + #expect(schedule.shouldRetry(afterElapsed: 0) == false) + } + + @Test func budgetRetriesUntilExhausted() { + let schedule = LockAcquireSchedule(budgetSeconds: 300) + #expect(schedule.shouldRetry(afterElapsed: 299)) + #expect(schedule.shouldRetry(afterElapsed: 300) == false) + #expect(schedule.shouldRetry(afterElapsed: 300.1) == false) + } + + @Test func stillWaitingAnnouncesOnceEveryInterval() { + let schedule = LockAcquireSchedule(budgetSeconds: 300) + #expect(schedule.shouldAnnounceStillWaiting(atElapsed: 0, lastAnnouncedElapsed: nil) == false) + #expect(schedule.shouldAnnounceStillWaiting(atElapsed: 14, lastAnnouncedElapsed: 0) == false) + #expect(schedule.shouldAnnounceStillWaiting(atElapsed: 15, lastAnnouncedElapsed: 0)) + #expect(schedule.shouldAnnounceStillWaiting(atElapsed: 16, lastAnnouncedElapsed: 15) == false) + #expect(schedule.shouldAnnounceStillWaiting(atElapsed: 30, lastAnnouncedElapsed: 15)) + } + + @Test func durationTextIsExact() { + #expect(DurationText.brief(seconds: 0) == "0s") + #expect(DurationText.brief(seconds: 59) == "59s") + #expect(DurationText.brief(seconds: 60) == "1m 00s") + #expect(DurationText.brief(seconds: 125) == "2m 05s") + #expect(DurationText.brief(seconds: 3720) == "1h 02m") + } +} diff --git a/Tests/DevCtlDaemonCoreTests/ResourceLockTests.swift b/Tests/DevCtlDaemonCoreTests/ResourceLockTests.swift index 33a138f..e0d9beb 100644 --- a/Tests/DevCtlDaemonCoreTests/ResourceLockTests.swift +++ b/Tests/DevCtlDaemonCoreTests/ResourceLockTests.swift @@ -115,6 +115,96 @@ private func phaseOf(router: Router, project: String, name: String) async throws expecting: ServerResult.self) } + /** A waiting run has to be able to name the holder, or it looks hung and + someone kills the run that is making progress. */ + @Test func lockStatusNamesTheLiveHolderAndForgetsADeadOne() async throws { + let env = try makeLockEnv() + try writeLockDevservers(project: env.projectPath) + let registry = Registry(paths: env.paths) + try await registry.setTrusted(project: env.projectPath) + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + try await startDB(router: router, project: env.projectPath) + + let empty = try await handle( + router: router, method: .lockStatus, + params: LockStatusParams(project: env.projectPath, resource: "data"), + expecting: LockStatusResult.self) + #expect(empty.holder == nil) + + _ = try await handle( + router: router, method: .lockAcquire, + params: LockParams( + holderPid: Int(getpid()), project: env.projectPath, resource: "data", + resumeTimeoutSeconds: 15), + expecting: LockResult.self) + let held = try await handle( + router: router, method: .lockStatus, + params: LockStatusParams(project: env.projectPath, resource: "data"), + expecting: LockStatusResult.self) + let holder = try #require(held.holder) + #expect(holder.pid == Int(getpid())) + #expect(holder.pause == true) + #expect(holder.paused == ["db"]) + #expect(holder.live == nil) + + _ = try await handle( + router: router, method: .lockRelease, + params: LockParams( + holderPid: Int(getpid()), project: env.projectPath, resource: "data", + resumeTimeoutSeconds: 15), + expecting: LockResult.self) + let after = try await handle( + router: router, method: .lockStatus, + params: LockStatusParams(project: env.projectPath, resource: "data"), + expecting: LockStatusResult.self) + #expect(after.holder == nil) + _ = try await handle( + router: router, method: .serverStop, + params: ServerTargetParams(name: "db", project: env.projectPath), + expecting: ServerResult.self) + } + + /** Under --no-pause the declarers stay up, and which ones is exactly what a + waiting run needs to be told. */ + @Test func noPauseAcquireRecordsTheServersItLeftRunning() async throws { + let env = try makeLockEnv() + try writeLockDevservers(project: env.projectPath) + let registry = Registry(paths: env.paths) + try await registry.setTrusted(project: env.projectPath) + let router = Router(launcher: SubprocessLauncher(), paths: env.paths, registry: registry) + try await startDB(router: router, project: env.projectPath) + + let acquired = try await handle( + router: router, method: .lockAcquire, + params: LockParams( + holderPid: Int(getpid()), pause: false, project: env.projectPath, + resource: "data", resumeTimeoutSeconds: 15), + expecting: LockResult.self) + #expect(acquired.live == ["db"]) + #expect(acquired.paused.isEmpty) + let held = try await handle( + router: router, method: .lockStatus, + params: LockStatusParams(project: env.projectPath, resource: "data"), + expecting: LockStatusResult.self) + #expect(held.holder?.live == ["db"]) + #expect(held.holder?.pause == false) + /** The claim is that nothing was paused. startDB does not health-gate, so + the server is legitimately still starting; `stopped` is what a pause + would have left behind. */ + #expect(try await phaseOf(router: router, project: env.projectPath, name: "db") != .stopped) + + _ = try await handle( + router: router, method: .lockRelease, + params: LockParams( + holderPid: Int(getpid()), project: env.projectPath, resource: "data", + resumeTimeoutSeconds: 15), + expecting: LockResult.self) + _ = try await handle( + router: router, method: .serverStop, + params: ServerTargetParams(name: "db", project: env.projectPath), + expecting: ServerResult.self) + } + /** The reported failure: daemon dies mid-hold, holder is gone, recover must resume the paused set from locks.json. */ @Test func recoverResumesWhenHolderIsDead() async throws { diff --git a/docs/cli-contract.md b/docs/cli-contract.md index 02ce98e..efbe1ee 100644 --- a/docs/cli-contract.md +++ b/docs/cli-contract.md @@ -75,7 +75,7 @@ Filled in per phase as each lands; golden tests reference the examples in this f - `devctl doctor [--fix] --json` → `{findings: [{detail, kind, severity}]}`: daemon/launchd state, captured-PATH staleness, the host:port signature table with conflicts, cross-project port collisions (`port-collision`: two unrelated projects declaring one port, which no host:port signature can catch because the hostnames differ while the bind does not; sibling worktrees are excluded since they rebind by design), unmanaged listeners on managed ports, and registry entries whose project path no longer exists (the daemon auto-prunes those on boot and machine-wide status; `--fix` remains an idempotent force path for leftovers). - `devctl switch [--no-fetch] [--timeout 120]` → clean-tree guard (refuses dirty; never stashes), fetch, group down, `git switch` (remote-tracking fallback), then the project's `lifecycle.switch` playbook (argv arrays run sequentially from the project root; failures stop with `devctl up` as the resume hint), then group up. Playbooks live in devservers.json `lifecycle` and are agent-configurable. - Config extras: project-level `icon` (project-relative path, per-server override) feeds Spotlight thumbnails; every server and head is indexed in Spotlight as ` · ` with subtitle `devctl · ` (best-effort; not a Top Hit launcher); `heads` and pins surface in the menu bar app. -- `devctl lock [--no-pause] [--acquire-timeout 300] [--timeout 120] -- ` → runs the command holding a project resource exclusively. By default the daemon pauses servers that declare the resource in their `locks` (devservers.json) and re-ensures them on release (even on command failure). `--no-pause` takes the mutex without stopping declarers (for harnesses that reuse the live server). `ensure`/`start` of a declaring server is refused (`resource-locked`, naming the holder pid) while a live holder owns it, regardless of `--no-pause`. Locks are path-scoped (`canonicalPath::resource`); they do not pause other checkouts. Locks persist across a daemon crash: a dead holder auto-releases and resumes the paused set; a still-live holder keeps them paused so the harness stays exclusive. Exit status is the command's. +- `devctl lock [--no-pause] [--acquire-timeout 300] [--timeout 120] -- ` → runs the command holding a project resource exclusively. By default the daemon pauses servers that declare the resource in their `locks` (devservers.json) and re-ensures them on release (even on command failure). `--no-pause` takes the mutex without stopping declarers (for harnesses that reuse the live server). `ensure`/`start` of a declaring server is refused (`resource-locked`, naming the holder pid) while a live holder owns it, regardless of `--no-pause`. Locks are path-scoped (`canonicalPath::resource`); they do not pause other checkouts. Locks persist across a daemon crash: a dead holder auto-releases and resumes the paused set; a still-live holder keeps them paused so the harness stays exclusive. Exit status is the command's. The `--` is required and devctl's own options go before it; everything after `--` is captured verbatim, so a nested `--`, a dash option, and an empty string all reach the command untouched. A missing terminator or an unknown option is rejected by the parser at exit 64 rather than being passed through. A contended acquire writes the holder's pid, how long it has been running, and what it paused or left running to stderr, then repeats a still-waiting line every 15s, so a wait is never silent; `--acquire-timeout 0` makes exactly one attempt and fails immediately with `resource-locked`. All of lock's own output is stderr: stdout belongs to the guarded command. - `devctl context`: the harness-agnostic session context: a fenced `` plain-text block (server phases, effective URLs, log paths, latent/rebound port-conflict warnings, the ensure/wait/why/logs/lock cheat-sheet) for the cwd's project. Linked worktrees get a banner naming the preferred host. Silent (exit 0) when the project is unregistered or untrusted or the daemon is down; never bootstraps; never contains raw log lines or command strings. - `devctl daemon status --json` → `{daemon?, launchd, reachable}`. `reachable` is whether the daemon answered over the socket, and it is the field to branch on: `launchd` reporting `running` only means a job is loaded, so a loaded-but-not-listening daemon prints a reassuring launchd line with `reachable: false`. `daemon` is present only when reachable. Exit stays 0 either way, because the launchd half is still a useful answer. - `devctl daemon install|uninstall [--purge]|start|stop|restart|status`: launchd lifecycle. `stop` drains and writes a deliberate-stop marker that auto-bootstrap honors; `restart` and `install` (upgrade) both capture running servers, bounce the daemon, and re-ensure them by name ("servers bounce, then come back"). The new daemon finishes `recoverAtStartup` before accepting socket clients, so that re-ensure cannot race a half-finished restore. `install` also stages-and-renames the daemon binary and captures the login-shell PATH into the agent plist. Reboot recovery: the LaunchAgent runs at load; starting a server records resume-on-boot; a machine shutdown drains without clearing it; `recoverAtStartup` resolves specs through the merged config+registry view (so committed `devservers.json` servers come back, not only ad-hoc `register` entries) and restores those servers one at a time so sibling port claims observe each other. A deliberate `devctl stop`/`down` clears the intent. Renamed or deleted servers leave orphan state rows that recover drops. diff --git a/scripts/smoke.sh b/scripts/smoke.sh index e72cdce..401887d 100755 --- a/scripts/smoke.sh +++ b/scripts/smoke.sh @@ -363,13 +363,42 @@ set +e NO_PAUSE_EXIT=$? set -e [[ "$NO_PAUSE_EXIT" -eq 0 ]] || fail "lock --no-pause failed ($NO_PAUSE_EXIT): $(head -c 400 "$NO_PAUSE_STATUS" 2>/dev/null) $(cat "$WORK/no-pause.err" 2>/dev/null)" -# Drop any human pause lines lock might print; the status JSON is the last object. -NO_PAUSE_PHASE="$(/usr/bin/python3 -c 'import json,sys; lines=open(sys.argv[1]).read().splitlines(); -objs=[json.loads(l) for l in lines if l.strip().startswith("{")]; -assert objs, open(sys.argv[1]).read(); -print(objs[-1]["servers"][0]["phase"])' "$NO_PAUSE_STATUS")" +# stdout belongs to the guarded command: lock's own chatter is on stderr, so +# this parses as plain JSON with nothing filtered out. +NO_PAUSE_PHASE="$(/usr/bin/python3 -c 'import json,sys; print(json.load(open(sys.argv[1]))["servers"][0]["phase"])' "$NO_PAUSE_STATUS")" [[ "$NO_PAUSE_PHASE" == "running" ]] || fail "lock --no-pause paused db (phase $NO_PAUSE_PHASE; out=$(cat "$NO_PAUSE_STATUS"))" pass "lock --no-pause leaves declarer running" + +# The parse defect: lock's own options after the resource joined the guarded +# command, so `env --timeout 20 -- sh` died with `env: illegal option -- t`. +"$DEVCTL" lock data --timeout 20 -- sh -c 'exit 0' 2>/dev/null || fail "lock leaked its own options into the guarded command" +pass "lock options before -- do not reach the guarded command" + +# A contended acquire has to name the holder rather than sit silent, and the +# fail-fast form must return at once instead of waiting out the budget. +"$DEVCTL" lock data -- sh -c 'sleep 4' >/dev/null 2>&1 & +HOLDER_JOB=$! +for _ in $(seq 1 60); do + if ! "$DEVCTL" lock data --acquire-timeout 0 -- true >/dev/null 2>&1; then break; fi +done +FAST_START=$SECONDS +set +e +"$DEVCTL" lock data --acquire-timeout 0 --json -- true > "$WORK/lockfast.json" 2>/dev/null +FAST_EXIT=$? +set -e +FAST_ELAPSED=$((SECONDS - FAST_START)) +[[ "$FAST_EXIT" -ne 0 ]] || fail "--acquire-timeout 0 acquired a held lock" +[[ "$FAST_ELAPSED" -lt 3 ]] || fail "--acquire-timeout 0 waited ${FAST_ELAPSED}s instead of failing fast" +/usr/bin/python3 -c "import json;d=json.load(open('$WORK/lockfast.json'));assert d['error']['code']=='resource-locked', d" || fail "fail-fast lock lost its error code" +set +e +"$DEVCTL" lock data --acquire-timeout 20 -- true 2>"$WORK/contended.err" >/dev/null +CONTENDED_EXIT=$? +set -e +wait $HOLDER_JOB 2>/dev/null || true +[[ "$CONTENDED_EXIT" -eq 0 ]] || fail "contended lock never acquired ($CONTENDED_EXIT): $(cat "$WORK/contended.err")" +grep -qE "is held by pid [0-9]+" "$WORK/contended.err" || fail "contended lock waited silently: $(cat "$WORK/contended.err")" +grep -q "waiting up to" "$WORK/contended.err" || fail "contended lock did not say the wait is bounded" +pass "contended lock names the holder and bounds the wait" "$DEVCTL" down --json > /dev/null # Deep links: print URL + dispatch via x-url (no Launch Services). From bc57d8ba7b489b2af66a4b3f64c98d9965a25861 Mon Sep 17 00:00:00 2001 From: Evan Jacobs Date: Fri, 7 Aug 2026 23:47:01 -0400 Subject: [PATCH 2/9] feat(lock): report a locked resource whose state changed under a live holder The incident this closes: a session wiped a local database directory to re-run migrations under --no-pause. The lock serialized access, the still-running server held the old file open and flushed its cached pages back over the migrated one, and the migration reported success while the seeded rows were gone. Three wrong diagnoses followed before the raw bytes settled it, because nothing in the output distinguished that run from a clean one. A locks entry may now name its state path, as an object beside the bare string form, which keeps parsing and re-encoding unchanged; that compatibility is the headline test. lock fingerprints the state around the guarded command and reports a change: a fault under --no-pause with a live declarer, a note otherwise. Inode is in the fingerprint because the incident's shape is a delete-and-recreate that a content hash calls identical. Two declarers naming different paths for one resource refuses rather than guessing which state a lock guards. The directory walk sorts before clipping so a truncated manifest stays deterministic, and it keeps each entry's own absolute path: rebuilding one from a stripped relative produced paths that existed nowhere, silently stat-failed, and left every capture comparing equal, which the entry-count assertion caught. What the check cannot catch is stated in the contract rather than implied. --- .changeset/lock-identity-guard.md | 7 + BACKLOG.md | 27 +-- .../Control/ControlServer.swift | 13 +- .../Supervisor/ServerSupervisor.swift | 2 +- Sources/DevCtlKit/Config/LockResource.swift | 42 ++++ Sources/DevCtlKit/Config/ProjectConfig.swift | 4 +- Sources/DevCtlKit/Model/Models.swift | 53 ++++- Sources/DevCtlKit/Paths/Paths.swift | 13 +- Sources/DevCtlKit/Protocol/Wire.swift | 10 +- .../DevCtlKit/Resource/ResourceIdentity.swift | 197 ++++++++++++++++++ Sources/devctl/CLI.swift | 91 +++++++- Tests/DevCtlCLITests/LockIdentityTests.swift | 91 ++++++++ Tests/DevCtlKitTests/ProjectConfigTests.swift | 51 +++++ .../ResourceIdentityTests.swift | 164 +++++++++++++++ docs/cli-contract.md | 4 +- scripts/smoke.sh | 47 ++++- 16 files changed, 771 insertions(+), 45 deletions(-) create mode 100644 .changeset/lock-identity-guard.md create mode 100644 Sources/DevCtlKit/Config/LockResource.swift create mode 100644 Sources/DevCtlKit/Resource/ResourceIdentity.swift create mode 100644 Tests/DevCtlCLITests/LockIdentityTests.swift create mode 100644 Tests/DevCtlKitTests/ResourceIdentityTests.swift diff --git a/.changeset/lock-identity-guard.md b/.changeset/lock-identity-guard.md new file mode 100644 index 0000000..8a72bea --- /dev/null +++ b/.changeset/lock-identity-guard.md @@ -0,0 +1,7 @@ +--- +"devctl": minor +--- + +`devctl lock` can now tell you when a command changed the state it was guarding while a server still held that state open. A `locks` entry may name where the resource lives on disk (`{"name": "d1", "path": ".wrangler/state/v3/d1"}`, alongside the plain `"d1"` form, which keeps working unchanged). With a path declared, `lock` fingerprints that state before and after the command. Under `--no-pause` with a declaring server still running, a change is a `resource-mutated` failure naming the servers to stop and the command to re-run, because the running server holds the old state open and can write its cached pages back over what the command wrote. Under the default paused mode the same change is just a note. + +This closes a silent data loss: a migration run under `--no-pause` that wiped and rebuilt a local database reported success while the seeded rows were gone, and nothing in the output distinguished that from a clean run. The check is deliberately modest about its limits, and the contract states them: it flags the risk window rather than the damage, it cannot see state outside the declared path or divergence that never reaches disk, and above 8 MiB it samples a file rather than hashing it whole. diff --git a/BACKLOG.md b/BACKLOG.md index fb257d7..c979d56 100644 --- a/BACKLOG.md +++ b/BACKLOG.md @@ -16,28 +16,5 @@ Open work only; entries are removed by the change that resolves them. - Split `CLI.swift`: every command struct lives in that one file, which is past the size where splitting is worth asking about. A structure decision, not drift; the codebase map describes the current layout. - Field-level config editing in the dashboard would preserve `devservers.json` formatting instead of normalizing writes. `devctl config init` writes indented JSON with sorted keys, so a recovered file and a dashboard-saved file now agree on shape, but a hand-authored one with its own ordering or comments still normalizes on save. -## `lock --no-pause` is not enough when the command deletes the locked state - -A session wiped a project's local database directory to re-run migrations from -scratch, under `devctl lock d1 --no-pause`. The lock serialized access, but the -dev server kept the file open across the deletion and flushed its cached pages -back over the freshly migrated file. The migration reported success, the ledger -recorded every file as applied, and the seeded rows were gone. Three separate -wrong diagnoses followed before reading the sqlite file's raw bytes settled it, -and one of those wrong diagnoses got as far as a new guardrail before being -disproved. - -`--no-pause` is documented as "the server tolerates staying up", which reads as a -property of the *server*. The property that actually matters is a property of the -*command*: whether it mutates the resource in place (fine) or removes and -recreates it (not fine, the open handle wins). - -Worth considering: - -- Refuse `--no-pause` when the command line touches the locked resource's own - state path with a removing verb (`rm`, `mv`, `rmdir`), or at least warn. -- Or make `lock` report, on completion, that the resource's backing file changed - identity (inode/hash) while a holder was up, which is the observable tell. - -Either turns a silent data loss into a loud refusal. Right now nothing in the -output distinguishes "migrated and seeded" from "migrated, seeded, and clobbered". +- Exact hashing above the file cap needs a streaming SHA-256. `SHA256Portable.digest` takes a whole `[UInt8]` with no incremental entry point, so a lock resource larger than 8 MiB is fingerprinted by head, tail, size, and mtime, and a middle-only rewrite that preserves all four escapes the identity check. The limit is asserted in `ResourceIdentityTests` and stated in the contract rather than left implicit. +- The identity check flags the risk window, not the damage: the flush that corrupts can land after the guarded command exits and the second capture is taken. Catching that would need the daemon to watch the resource across the whole hold, or to compare again once the paused set has resumed. diff --git a/Sources/DevCtlDaemonCore/Control/ControlServer.swift b/Sources/DevCtlDaemonCore/Control/ControlServer.swift index 765f804..87e35b9 100644 --- a/Sources/DevCtlDaemonCore/Control/ControlServer.swift +++ b/Sources/DevCtlDaemonCore/Control/ControlServer.swift @@ -1076,7 +1076,8 @@ public actor Router { var paused: [String] = [] let shouldPause = params.pause ?? true let merged = try? await mergedSpecs(project: params.project) - for spec in merged?.specs ?? [] where (spec.locks ?? []).contains(params.resource) { + for spec in merged?.specs ?? [] + where LockResource.declares(resource: params.resource, spec: spec) { let supervisor = await supervisor(project: params.project, spec: spec) let status = await supervisor.status() switch status.phase { @@ -1100,11 +1101,16 @@ public actor Router { } live.sort() paused.sort() + /** Refusing here is correct when devctl cannot tell which state the lock + guards: taking it anyway would report on the wrong file. */ + let statePath = try LockResource.statePath( + project: params.project, resource: params.resource, specs: merged?.specs ?? []) resourceLocks[key] = LockHolder( live: live.isEmpty ? nil : live, pause: shouldPause, paused: paused, pid: params.holderPid, resumeTimeoutSeconds: params.resumeTimeoutSeconds, since: Date()) persistLocks() - return LockResult(live: live.isEmpty ? nil : live, paused: paused) + return LockResult( + live: live.isEmpty ? nil : live, paused: paused, statePath: statePath) } /** Who holds a resource right now, if anyone. A dead holder is released @@ -1220,7 +1226,8 @@ public actor Router { declared resources: restarting mid-harness-run is exactly the contention the lock exists to prevent. */ private func lockGate(project: String, spec: ServerSpec) async throws { - for resource in spec.locks ?? [] { + for declaration in spec.locks ?? [] { + let resource = declaration.name let key = Self.lockKey(project: project, resource: resource) await releaseOrphanedLock(key: key) if let holder = resourceLocks[key] { diff --git a/Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift b/Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift index 7566f42..5684d6c 100644 --- a/Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift +++ b/Sources/DevCtlDaemonCore/Supervisor/ServerSupervisor.swift @@ -304,7 +304,7 @@ public actor ServerSupervisor { icon: spec.icon, lastExit: lastExit, lastHealthAt: lastHealthAt, - locks: spec.locks, + locks: spec.locks.map { $0.map(\.name) }, logPath: paths.structuredLogFile(project: projectPath, server: spec.name).path, observedPort: observedPort, phase: phase, diff --git a/Sources/DevCtlKit/Config/LockResource.swift b/Sources/DevCtlKit/Config/LockResource.swift new file mode 100644 index 0000000..5ca4ba4 --- /dev/null +++ b/Sources/DevCtlKit/Config/LockResource.swift @@ -0,0 +1,42 @@ +import Foundation + +/** Reading a resource's declarations across the servers of one project. Kept in + one place so the pause loop, the lock gate, and the identity check cannot + disagree about which servers declare a resource or where its state lives. */ +public enum LockResource { + public static func declares(resource: String, spec: ServerSpec) -> Bool { + (spec.locks ?? []).contains { $0.name == resource } + } + + public static func declarers(resource: String, specs: [ServerSpec]) -> [String] { + specs.filter { declares(resource: resource, spec: $0) }.map(\.name).sorted() + } + + /** The absolute state path declared for `resource`, or nil when no declarer + names one. Throws when two declarers name different paths: devctl would + have to guess which state it is guarding, and guessing is how the + incident behind this check happened. */ + public static func statePath(project: String, resource: String, specs: [ServerSpec]) throws + -> String? + { + var found: (path: String, server: String)? + for spec in specs { + for declaration in spec.locks ?? [] + where declaration.name == resource && declaration.path != nil { + guard let relative = declaration.path else { continue } + let absolute = URL(fileURLWithPath: project).appending(path: relative) + .standardizedFileURL.path + if let existing = found, existing.path != absolute { + throw WireError( + code: .configInvalid, + hint: "run: devctl config check", + message: + "servers '\(existing.server)' and '\(spec.name)' declare resource '\(resource)' with different state paths (\(existing.path) and \(absolute)); devctl cannot tell which state a lock guards" + ) + } + found = (path: absolute, server: spec.name) + } + } + return found?.path + } +} diff --git a/Sources/DevCtlKit/Config/ProjectConfig.swift b/Sources/DevCtlKit/Config/ProjectConfig.swift index 5273328..3f12fd0 100644 --- a/Sources/DevCtlKit/Config/ProjectConfig.swift +++ b/Sources/DevCtlKit/Config/ProjectConfig.swift @@ -41,7 +41,7 @@ public struct ProjectFileServer: Codable, Equatable, Sendable { public var healthcheck: HealthCheckSpec? public var host: String? public var icon: String? - public var locks: [String]? + public var locks: [LockDeclaration]? public var port: Int? public var portEnv: String? public var ports: [String: SecondaryPort]? @@ -59,7 +59,7 @@ public struct ProjectFileServer: Codable, Equatable, Sendable { healthcheck: HealthCheckSpec? = nil, host: String? = nil, icon: String? = nil, - locks: [String]? = nil, + locks: [LockDeclaration]? = nil, port: Int? = nil, portEnv: String? = nil, ports: [String: SecondaryPort]? = nil, diff --git a/Sources/DevCtlKit/Model/Models.swift b/Sources/DevCtlKit/Model/Models.swift index 1435e55..79e3c3b 100644 --- a/Sources/DevCtlKit/Model/Models.swift +++ b/Sources/DevCtlKit/Model/Models.swift @@ -61,6 +61,55 @@ public enum WaitTarget: String, Codable, Sendable { case started } +/** A resource a server holds while running. Written either as a bare name + (`"d1"`) or as an object naming where the resource's state lives on disk + (`{"name": "d1", "path": ".wrangler/state/v3/d1"}`). Both forms decode and the + bare form re-encodes bare, so existing registry entries and committed configs + never churn. The path is what lets `devctl lock` notice that a command + changed the state while a declaring server still held the old file open. */ +public struct LockDeclaration: Codable, Equatable, Hashable, Sendable { + public var name: String + /** Project-relative path to the resource's state, a file or a directory. + Absent means devctl cannot check identity for this resource, and will not + pretend to. */ + public var path: String? + + public init(name: String, path: String? = nil) { + self.name = name + self.path = path + } + + private enum CodingKeys: String, CodingKey { + case name + case path + } + + public init(from decoder: any Decoder) throws { + /** Probing the string form is the discriminator between the two shapes, + not a swallowed error: the object branch reports its own decode + failure with the real key path. */ + if let name = try? decoder.singleValueContainer().decode(String.self) { + self.name = name + self.path = nil + return + } + let keyed = try decoder.container(keyedBy: CodingKeys.self) + self.name = try keyed.decode(String.self, forKey: .name) + self.path = try keyed.decodeIfPresent(String.self, forKey: .path) + } + + public func encode(to encoder: any Encoder) throws { + guard let path else { + var single = encoder.singleValueContainer() + try single.encode(name) + return + } + var keyed = encoder.container(keyedBy: CodingKeys.self) + try keyed.encode(name, forKey: .name) + try keyed.encode(path, forKey: .path) + } +} + /** What `devctl wait` blocks on. */ public enum WaitCondition: String, Codable, Sendable { case healthy @@ -108,7 +157,7 @@ public struct ServerSpec: Codable, Equatable, Sendable { database, a fixture directory). `devctl lock -- cmd` stops holders for the command's duration, and starts are refused while a live external holder owns the resource. */ - public var locks: [String]? + public var locks: [LockDeclaration]? public var name: String public var port: Int? /** Child env var that receives the effective port (default `PORT`). */ @@ -132,7 +181,7 @@ public struct ServerSpec: Codable, Equatable, Sendable { healthcheck: HealthCheckSpec? = nil, host: String? = nil, icon: String? = nil, - locks: [String]? = nil, + locks: [LockDeclaration]? = nil, name: String, port: Int? = nil, portEnv: String? = nil, diff --git a/Sources/DevCtlKit/Paths/Paths.swift b/Sources/DevCtlKit/Paths/Paths.swift index 690e6f3..c3a2c99 100644 --- a/Sources/DevCtlKit/Paths/Paths.swift +++ b/Sources/DevCtlKit/Paths/Paths.swift @@ -69,13 +69,18 @@ public struct DevCtlPaths: Sendable { serverLogDir(project: project, server: server).appending(path: "current.log") } - /** First 8 hex chars of SHA-256 over the canonical project path. */ - public static func hash8(_ string: String) -> String { - SHA256Portable.digest(Array(string.utf8)) - .prefix(4) + /** Full hex SHA-256. `hash8` is its first 8 characters and stays so: log + directory names on disk derive from that prefix. */ + public static func hashHex(_ bytes: [UInt8]) -> String { + SHA256Portable.digest(bytes) .map { String(format: "%02x", $0) } .joined() } + + /** First 8 hex chars of SHA-256 over the canonical project path. */ + public static func hash8(_ string: String) -> String { + String(hashHex(Array(string.utf8)).prefix(8)) + } } /** The human-readable half of a project's on-disk and host identity: the last diff --git a/Sources/DevCtlKit/Protocol/Wire.swift b/Sources/DevCtlKit/Protocol/Wire.swift index 5889ffc..382e118 100644 --- a/Sources/DevCtlKit/Protocol/Wire.swift +++ b/Sources/DevCtlKit/Protocol/Wire.swift @@ -77,13 +77,14 @@ public enum WireErrorCode: String, Codable, Sendable { case portDrift = "port-drift" case portHeld = "port-held" case resourceLocked = "resource-locked" + case resourceMutated = "resource-mutated" case spawnFailed = "spawn-failed" case usage case versionMismatch = "version-mismatch" } /** Wire and CLI error shape; `hint` is the literal remediation command when one exists. */ -public struct WireError: Codable, Error, Sendable { +public struct WireError: Codable, Equatable, Error, Sendable { public var code: WireErrorCode public var hint: String? public var message: String @@ -382,10 +383,15 @@ public struct LockResult: Codable, Equatable, Sendable { /** Declaring servers this hold left running, under `--no-pause`. */ public var live: [String]? public var paused: [String] + /** Absolute path to the resource's declared state, when a declarer names + one. The daemon resolves it because it already holds the merged view; + a second resolution in the CLI could disagree with the pause loop's. */ + public var statePath: String? - public init(live: [String]? = nil, paused: [String] = []) { + public init(live: [String]? = nil, paused: [String] = [], statePath: String? = nil) { self.live = live self.paused = paused + self.statePath = statePath } } diff --git a/Sources/DevCtlKit/Resource/ResourceIdentity.swift b/Sources/DevCtlKit/Resource/ResourceIdentity.swift new file mode 100644 index 0000000..906727e --- /dev/null +++ b/Sources/DevCtlKit/Resource/ResourceIdentity.swift @@ -0,0 +1,197 @@ +import Foundation + +public enum ResourceKind: String, Codable, Sendable { + case directory + case file + case missing + case symlink +} + +/** A cheap, deterministic fingerprint of a lock resource's on-disk state, taken + around a guarded command so a change made while a declaring server still held + the old file open can be reported rather than silently accepted. */ +public struct ResourceIdentity: Codable, Equatable, Sendable { + public var bytes: Int64 + /** Hex SHA-256 over the sampled manifest. The manifest's exact line format + is part of what this digest means; see ResourceFingerprint. */ + public var digest: String + public var entryCount: Int + /** Every sampled byte is inside the digest. False when a size or budget cap + forced head-and-tail sampling, which is why a report says "sampled". */ + public var exact: Bool + /** `:` of the root. A replaced file or a recreated directory + changes this even when the bytes match, which is the case a content hash + alone cannot see and the one that caused the incident. */ + public var inode: String + public var kind: ResourceKind + /** The entry cap clipped a directory walk. */ + public var truncated: Bool + + public init( + bytes: Int64 = 0, digest: String = "", entryCount: Int = 0, exact: Bool = true, + inode: String = "", kind: ResourceKind, truncated: Bool = false + ) { + self.bytes = bytes + self.digest = digest + self.entryCount = entryCount + self.exact = exact + self.inode = inode + self.kind = kind + self.truncated = truncated + } +} + +/** Why two identities differ, in the order a reader wants to hear it. */ +public enum ResourceChangeReason: String, Sendable { + case content + case inode + case kind + case size +} + +public enum ResourceChange: Equatable, Sendable { + case appeared + case changed(ResourceChangeReason) + case disappeared + case unchanged +} + +public enum ResourceFingerprint { + /** Files at or below this hash whole; larger ones contribute head, tail, + size, and mtime. SHA256Portable takes a whole `[UInt8]` with no streaming + entry point, so an exact digest of a large file costs a full buffer. */ + public static let fileByteCap = 8 << 20 + public static let sampleWindowBytes = 1 << 20 + public static let directoryContentBudget = 8 << 20 + public static let maxDepth = 6 + public static let maxEntries = 4096 + + public static func capture(path: String) -> ResourceIdentity { + var info = stat() + guard lstat(path, &info) == 0 else { return ResourceIdentity(kind: .missing) } + let inode = "\(info.st_dev):\(info.st_ino)" + if info.st_mode & S_IFMT == S_IFLNK { + let target = (try? FileManager.default.destinationOfSymbolicLink(atPath: path)) ?? "" + return ResourceIdentity( + bytes: Int64(info.st_size), digest: DevCtlPaths.hashHex(Array(target.utf8)), + entryCount: 1, inode: inode, kind: .symlink) + } + if info.st_mode & S_IFMT == S_IFDIR { + return captureDirectory(inode: inode, path: path) + } + let sample = sampleFile(path: path, size: Int64(info.st_size), stat: info) + return ResourceIdentity( + bytes: Int64(info.st_size), digest: DevCtlPaths.hashHex(sample.bytes), entryCount: 1, + exact: sample.exact, inode: inode, kind: .file) + } + + public static func compare(after: ResourceIdentity, before: ResourceIdentity) + -> ResourceChange + { + if before.kind == .missing, after.kind == .missing { return .unchanged } + if before.kind == .missing { return .appeared } + if after.kind == .missing { return .disappeared } + if before.kind != after.kind { return .changed(.kind) } + if before.inode != after.inode { return .changed(.inode) } + if before.bytes != after.bytes { return .changed(.size) } + if before.digest != after.digest { return .changed(.content) } + return .unchanged + } + + /** A directory of sqlite files is the shape this exists for, and the caps + are what keep a `path` aimed at a large tree from making every lock cost + a full walk. */ + private static func captureDirectory(inode: String, path: String) -> ResourceIdentity { + /** The enumerator hands back resolved paths (`/private/var/...` for a + `/var/...` root), so the root is resolved too and each item's own + absolute path is kept rather than rebuilt. Re-joining a stripped + relative onto the unresolved root produced paths that existed nowhere + and silently stat-failed, leaving an empty manifest that compared + equal to every other empty one. */ + let root = URL(fileURLWithPath: path).resolvingSymlinksInPath() + let prefix = root.path.hasSuffix("/") ? root.path : root.path + "/" + var entries: [(absolute: String, relative: String)] = [] + var clipped = false + if let walker = FileManager.default.enumerator( + at: root, includingPropertiesForKeys: nil, + options: [.skipsHiddenFiles, .skipsPackageDescendants]) + { + for case let item as URL in walker { + guard walker.level <= maxDepth else { + walker.skipDescendants() + continue + } + let absolute = item.path + let relative = + absolute.hasPrefix(prefix) + ? String(absolute.dropFirst(prefix.count)) : item.lastPathComponent + entries.append((absolute: absolute, relative: relative)) + if entries.count > maxEntries * 4 { + clipped = true + break + } + } + } + /** Sort before clipping so a truncated manifest is still deterministic: + the enumerator's own order is unspecified, and clipping during the + walk would make two captures of one tree disagree. */ + entries.sort { $0.relative.utf8.lexicographicallyPrecedes($1.relative.utf8) } + if entries.count > maxEntries { + entries = Array(entries.prefix(maxEntries)) + clipped = true + } + var budget = directoryContentBudget + var exact = !clipped + var bytes: Int64 = 0 + var lines: [String] = [] + for entry in entries { + let full = entry.absolute + let relative = entry.relative + var info = stat() + guard lstat(full, &info) == 0 else { continue } + bytes += Int64(info.st_size) + let mtime = Int64(info.st_mtimespec.tv_sec) * 1_000_000_000 + + Int64(info.st_mtimespec.tv_nsec) + var content = "-" + if info.st_mode & S_IFMT == S_IFREG { + if budget >= Int(info.st_size) { + let sample = sampleFile(path: full, size: Int64(info.st_size), stat: info) + content = DevCtlPaths.hashHex(sample.bytes) + budget -= Int(info.st_size) + exact = exact && sample.exact + } else { + exact = false + } + } + lines.append( + "\(relative)\t\(info.st_dev):\(info.st_ino)\t\(info.st_size)\t\(mtime)\t\(content)") + } + return ResourceIdentity( + bytes: bytes, digest: DevCtlPaths.hashHex(Array(lines.joined(separator: "\n").utf8)), + entryCount: lines.count, exact: exact, inode: inode, kind: .directory, + truncated: clipped) + } + + /** Whole contents up to the cap, else head plus tail plus size and mtime. + Above the cap a middle-only rewrite that preserves head, tail, size, and + mtime is invisible; the identity says so through `exact`. */ + private static func sampleFile(path: String, size: Int64, stat info: stat) -> ( + bytes: [UInt8], exact: Bool + ) { + guard let handle = FileHandle(forReadingAtPath: path) else { return ([], false) } + defer { try? handle.close() } + if size <= Int64(fileByteCap) { + let data = (try? handle.readToEnd()) ?? Data() + return (Array(data), true) + } + let head = (try? handle.read(upToCount: sampleWindowBytes)) ?? Data() + try? handle.seek(toOffset: UInt64(max(size - Int64(sampleWindowBytes), 0))) + let tail = (try? handle.read(upToCount: sampleWindowBytes)) ?? Data() + let mtime = Int64(info.st_mtimespec.tv_sec) * 1_000_000_000 + + Int64(info.st_mtimespec.tv_nsec) + var bytes = Array(head) + bytes.append(contentsOf: Array(tail)) + bytes.append(contentsOf: Array("\(size)\t\(mtime)".utf8)) + return (bytes, false) + } +} diff --git a/Sources/devctl/CLI.swift b/Sources/devctl/CLI.swift index fd7975f..744eae3 100644 --- a/Sources/devctl/CLI.swift +++ b/Sources/devctl/CLI.swift @@ -79,7 +79,9 @@ enum CLIRunner { } } - static func fail(_ error: WireError, json: Bool) -> Never { + /** Render the failure without deciding the exit status, so a caller that has + its own status to honor (a guarded command's) can still report. */ + static func emitFailure(_ error: WireError, json: Bool) { if json { struct Envelope: Codable { var error: WireError @@ -93,6 +95,10 @@ enum CLIRunner { if let hint = error.hint { text += "\n \(hint)" } FileHandle.standardError.write(Data((text + "\n").utf8)) } + } + + static func fail(_ error: WireError, json: Bool) -> Never { + emitFailure(error, json: json) switch error.code { case .daemonUnreachable, .versionMismatch: Foundation.exit(3) @@ -101,7 +107,7 @@ enum CLIRunner { case .usage: Foundation.exit(2) case .alreadyExists, .configInvalid, .internalError, .notTrusted, .portDrift, .portHeld, - .resourceLocked, .spawnFailed: + .resourceLocked, .resourceMutated, .spawnFailed: Foundation.exit(1) } } @@ -1616,6 +1622,67 @@ enum LockNotice { } } +/** What the identity check concluded about the locked state. + + What it cannot catch, stated so nobody over-reads it: it flags the risk + window, not the damage, because the incident's corruption landed when the + still-running server flushed its cached pages after the command had already + finished. It cannot see state outside the declared path (a sibling `-wal` + file when `path` names only the `.sqlite`), divergence that never reaches + disk, or a change that reverts to byte-identical state inside the window. + Above the file cap it samples head and tail, so a middle-only rewrite that + preserves size and mtime is missed. It never names which process wrote. */ +enum LockIdentityVerdict: Equatable { + case fault(WireError) + case note(String) + case silent + + /** Under `--no-pause` a live declarer holds the old file open, so any change + to the locked state during the hold is not durable whatever the command + reported. Under the default paused mode the same change is the entire + point, so it is a note. */ + static func of( + after: ResourceIdentity, before: ResourceIdentity, live: [String], resource: String, + statePath: String + ) -> LockIdentityVerdict { + let change = ResourceFingerprint.compare(after: after, before: before) + guard change != .unchanged else { return .silent } + let described = describe(change) + guard !live.isEmpty else { + return .note( + "devctl lock: note: '\(resource)' state at \(statePath) changed during this hold (\(described)). Nothing was running against it." + ) + } + let servers = live.sorted() + return .fault( + WireError( + code: .resourceMutated, + hint: "devctl stop \(servers.joined(separator: " && devctl stop ")) && devctl lock \(resource) -- && devctl ensure \(servers.joined(separator: " && devctl ensure "))", + message: + "resource '\(resource)' state at \(statePath) changed (\(described)) while \(servers.joined(separator: ", ")) stayed running under --no-pause. That server holds the old state open and can write its cached pages back over the change, so what is on disk is not what the command wrote." + )) + } + + private static func describe(_ change: ResourceChange) -> String { + switch change { + case .appeared: + return "it was created" + case .changed(.content): + return "its contents differ" + case .changed(.inode): + return "it was replaced" + case .changed(.kind): + return "it changed kind" + case .changed(.size): + return "its size changed" + case .disappeared: + return "it was removed" + case .unchanged: + return "unchanged" + } + } +} + /** Compact human durations. Nothing else in the CLI formats one. */ enum DurationText { static func brief(seconds: Double) -> String { @@ -1745,6 +1812,9 @@ struct Lock: AsyncParsableCommand { for name in acquired.paused { Self.note("devctl lock: paused \(name) (holds \(resource))") } + /** Identity is taken before the command and again before release, so a + resumed server's first writes are never blamed on the command. */ + let before = acquired.statePath.map(ResourceFingerprint.capture(path:)) /** Run the guarded command with inherited stdio. */ let process = Process() process.executableURL = URL(fileURLWithPath: "/usr/bin/env") @@ -1758,6 +1828,12 @@ struct Lock: AsyncParsableCommand { } catch { FileHandle.standardError.write(Data("devctl lock: cannot run command: \(error)\n".utf8)) } + var verdict = LockIdentityVerdict.silent + if let statePath = acquired.statePath, let before { + verdict = LockIdentityVerdict.of( + after: ResourceFingerprint.capture(path: statePath), before: before, + live: acquired.live ?? [], resource: resource, statePath: statePath) + } /** Release resumes whoever was paused, even if the command failed. */ let released = (try? await client.request( .lockRelease, @@ -1768,6 +1844,17 @@ struct Lock: AsyncParsableCommand { for name in released.paused { Self.note("devctl lock: resuming \(name)…") } + switch verdict { + case .fault(let error): + /** A failing command keeps its own status: never swallow that. A + clean command that silently lost its work exits 1. */ + CLIRunner.emitFailure(error, json: global.json) + Foundation.exit(commandStatus == 0 ? 1 : commandStatus) + case .note(let text): + Self.note(text) + case .silent: + break + } Foundation.exit(commandStatus) } diff --git a/Tests/DevCtlCLITests/LockIdentityTests.swift b/Tests/DevCtlCLITests/LockIdentityTests.swift new file mode 100644 index 0000000..cd1be33 --- /dev/null +++ b/Tests/DevCtlCLITests/LockIdentityTests.swift @@ -0,0 +1,91 @@ +import DevCtlKit +import Foundation +import Testing + +@testable import devctl + +/** The incident: a session wiped a local database directory to re-run migrations + under `--no-pause`. The lock serialized access, the still-running server held + the old file open and flushed its cached pages back over the migrated one, and + the migration reported success while the seeded rows were gone. Nothing in the + output distinguished that from a clean run. */ +@Suite struct LockIdentityTests { + private let file = ResourceIdentity( + bytes: 10, digest: "aaa", entryCount: 1, inode: "1:2", kind: .file) + + @Test func changedUnderNoPauseWithALiveServerIsAFault() throws { + let after = ResourceIdentity( + bytes: 10, digest: "aaa", entryCount: 1, inode: "1:9", kind: .file) + let verdict = LockIdentityVerdict.of( + after: after, before: file, live: ["db"], resource: "d1", + statePath: "/p/state") + guard case .fault(let error) = verdict else { + Issue.record("expected a fault, got \(verdict)") + return + } + #expect(error.code == .resourceMutated) + #expect( + error.message + == "resource 'd1' state at /p/state changed (it was replaced) while db stayed running under --no-pause. That server holds the old state open and can write its cached pages back over the change, so what is on disk is not what the command wrote." + ) + #expect( + error.hint == "devctl stop db && devctl lock d1 -- && devctl ensure db") + } + + @Test func theFaultHintListsEveryLiveServerSorted() throws { + let after = ResourceIdentity( + bytes: 11, digest: "bbb", entryCount: 1, inode: "1:2", kind: .file) + let verdict = LockIdentityVerdict.of( + after: after, before: file, live: ["web", "db"], resource: "d1", statePath: "/p/s") + guard case .fault(let error) = verdict else { + Issue.record("expected a fault") + return + } + #expect( + error.hint + == "devctl stop db && devctl stop web && devctl lock d1 -- && devctl ensure db && devctl ensure web") + } + + /** Under the default paused mode a change is the entire point, so it is + information rather than a fault. */ + @Test func changedWithNothingRunningIsANote() { + let after = ResourceIdentity( + bytes: 12, digest: "ccc", entryCount: 1, inode: "1:2", kind: .file) + let verdict = LockIdentityVerdict.of( + after: after, before: file, live: [], resource: "d1", statePath: "/p/state") + #expect( + verdict + == .note( + "devctl lock: note: 'd1' state at /p/state changed during this hold (its size changed). Nothing was running against it." + )) + } + + @Test func unchangedStateIsSilent() { + #expect( + LockIdentityVerdict.of( + after: file, before: file, live: ["db"], resource: "d1", statePath: "/p/s") + == .silent) + } + + @Test func aRemovedResourceUnderNoPauseReadsAsRemoved() throws { + let gone = ResourceIdentity(kind: .missing) + let verdict = LockIdentityVerdict.of( + after: gone, before: file, live: ["db"], resource: "d1", statePath: "/p/s") + guard case .fault(let error) = verdict else { + Issue.record("expected a fault") + return + } + #expect(error.message.contains("(it was removed)")) + } + + @Test func aCreatedResourceUnderNoPauseReadsAsCreated() throws { + let verdict = LockIdentityVerdict.of( + after: file, before: ResourceIdentity(kind: .missing), live: ["db"], resource: "d1", + statePath: "/p/s") + guard case .fault(let error) = verdict else { + Issue.record("expected a fault") + return + } + #expect(error.message.contains("(it was created)")) + } +} diff --git a/Tests/DevCtlKitTests/ProjectConfigTests.swift b/Tests/DevCtlKitTests/ProjectConfigTests.swift index 4170143..2690c92 100644 --- a/Tests/DevCtlKitTests/ProjectConfigTests.swift +++ b/Tests/DevCtlKitTests/ProjectConfigTests.swift @@ -172,6 +172,57 @@ import Testing }) } + /** The compatibility gate for the locks schema: every config and registry + file written before a lock could name a path uses the bare string form, + and must keep parsing and re-encoding unchanged. */ + @Test func aBareStringLockStillParsesAndReEncodesBare() throws { + let json = #"{"command":["x"],"locks":["d1","cache"],"name":"web"}"# + let spec = try JSONCoding.decoder().decode(ServerSpec.self, from: Data(json.utf8)) + #expect(spec.locks == [LockDeclaration(name: "d1"), LockDeclaration(name: "cache")]) + let encoded = String( + data: try JSONCoding.encoder().encode(spec), encoding: .utf8) + #expect(encoded == #"{"command":["x"],"locks":["d1","cache"],"name":"web"}"#) + } + + @Test func aPathedLockRoundTripsAsAnObject() throws { + let json = #"{"command":["x"],"locks":[{"name":"d1","path":"state/v3"}],"name":"web"}"# + let spec = try JSONCoding.decoder().decode(ServerSpec.self, from: Data(json.utf8)) + #expect(spec.locks == [LockDeclaration(name: "d1", path: "state/v3")]) + let encoded = String(data: try JSONCoding.encoder().encode(spec), encoding: .utf8) + #expect(encoded == json) + } + + @Test func bothLockFormsMayAppearInOneArray() throws { + let json = #"{"command":["x"],"locks":["cache",{"name":"d1","path":"state"}],"name":"web"}"# + let spec = try JSONCoding.decoder().decode(ServerSpec.self, from: Data(json.utf8)) + #expect( + spec.locks == [LockDeclaration(name: "cache"), LockDeclaration(name: "d1", path: "state")]) + } + + @Test func statePathResolvesAgainstTheProjectRoot() throws { + let specs = [ + ServerSpec(command: ["x"], locks: [LockDeclaration(name: "d1", path: "state/v3")], name: "web"), + ServerSpec(command: ["x"], locks: [LockDeclaration(name: "d1")], name: "api"), + ] + #expect( + try LockResource.statePath(project: "/Users/x/proj", resource: "d1", specs: specs) + == "/Users/x/proj/state/v3") + #expect(try LockResource.statePath(project: "/p", resource: "cache", specs: specs) == nil) + #expect(LockResource.declarers(resource: "d1", specs: specs) == ["api", "web"]) + } + + /** Guessing which state a lock guards is how the incident behind the + identity check happened, so a disagreement refuses instead. */ + @Test func conflictingStatePathsForOneResourceIsAConfigError() { + let specs = [ + ServerSpec(command: ["x"], locks: [LockDeclaration(name: "d1", path: "a")], name: "api"), + ServerSpec(command: ["x"], locks: [LockDeclaration(name: "d1", path: "b")], name: "web"), + ] + #expect(throws: WireError.self) { + _ = try LockResource.statePath(project: "/p", resource: "d1", specs: specs) + } + } + @Test func parseErrorIsActionable() throws { let dir = FileManager.default.temporaryDirectory.appending(path: "devctl-cfg-\(UUID().uuidString)") try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) diff --git a/Tests/DevCtlKitTests/ResourceIdentityTests.swift b/Tests/DevCtlKitTests/ResourceIdentityTests.swift new file mode 100644 index 0000000..a8eabaf --- /dev/null +++ b/Tests/DevCtlKitTests/ResourceIdentityTests.swift @@ -0,0 +1,164 @@ +import Foundation +import Testing + +@testable import DevCtlKit + +@Suite struct ResourceIdentityTests { + private func scratch() throws -> URL { + let dir = FileManager.default.temporaryDirectory + .appending(path: "devctl-res-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: dir, withIntermediateDirectories: true) + return dir + } + + @Test func hash8IsUnchangedByTheHashHexRefactor() { + /** Log directory names on disk derive from this prefix, so it is pinned: + a change here moves every project's log path. */ + #expect(DevCtlPaths.hash8("/Users/x/code/shop") == String(DevCtlPaths.hashHex(Array("/Users/x/code/shop".utf8)).prefix(8))) + #expect(DevCtlPaths.hash8("").count == 8) + #expect(DevCtlPaths.hashHex([]).count == 64) + } + + @Test func aContentChangeAtEqualSizeIsDetected() throws { + let dir = try scratch() + let file = dir.appending(path: "db.sqlite") + try Data("aaaa".utf8).write(to: file) + let before = ResourceFingerprint.capture(path: file.path) + try Data("bbbb".utf8).write(to: file) + let after = ResourceFingerprint.capture(path: file.path) + #expect(before.bytes == after.bytes) + #expect(ResourceFingerprint.compare(after: after, before: before) != .unchanged) + } + + /** The incident's shape: the file is removed and recreated, so a content + hash alone can call it unchanged while the open handle still wins. */ + @Test func replacingAFileWithIdenticalBytesIsDetected() throws { + let dir = try scratch() + let file = dir.appending(path: "db.sqlite") + try Data("same".utf8).write(to: file) + let before = ResourceFingerprint.capture(path: file.path) + try FileManager.default.removeItem(at: file) + try Data("same".utf8).write(to: file) + let after = ResourceFingerprint.capture(path: file.path) + #expect(before.digest == after.digest) + #expect(ResourceFingerprint.compare(after: after, before: before) == .changed(.inode)) + } + + @Test func repeatedCaptureOfAnUnchangedDirectoryIsIdentical() throws { + let dir = try scratch() + for name in ["b.sqlite", "a.sqlite", "Z.sqlite", "é.sqlite"] { + try Data(name.utf8).write(to: dir.appending(path: name)) + } + let first = ResourceFingerprint.capture(path: dir.path) + let second = ResourceFingerprint.capture(path: dir.path) + #expect(first == second) + #expect(ResourceFingerprint.compare(after: second, before: first) == .unchanged) + #expect(first.kind == .directory) + #expect(first.entryCount == 4) + } + + @Test func directoryChangesOnAddRemoveAndModify() throws { + let dir = try scratch() + try Data("one".utf8).write(to: dir.appending(path: "a.sqlite")) + let base = ResourceFingerprint.capture(path: dir.path) + + try Data("two".utf8).write(to: dir.appending(path: "b.sqlite")) + let added = ResourceFingerprint.capture(path: dir.path) + #expect(ResourceFingerprint.compare(after: added, before: base) != .unchanged) + + try Data("changed".utf8).write(to: dir.appending(path: "a.sqlite")) + let modified = ResourceFingerprint.capture(path: dir.path) + #expect(ResourceFingerprint.compare(after: modified, before: added) != .unchanged) + + try FileManager.default.removeItem(at: dir.appending(path: "b.sqlite")) + let removed = ResourceFingerprint.capture(path: dir.path) + #expect(ResourceFingerprint.compare(after: removed, before: modified) != .unchanged) + } + + /** A d1 state directory is wiped and rebuilt, which is exactly the incident. */ + @Test func aRecreatedDirectoryIsDetectedEvenWithIdenticalContents() throws { + let dir = try scratch() + let state = dir.appending(path: "state") + try FileManager.default.createDirectory(at: state, withIntermediateDirectories: true) + try Data("rows".utf8).write(to: state.appending(path: "db.sqlite")) + let before = ResourceFingerprint.capture(path: state.path) + try FileManager.default.removeItem(at: state) + try FileManager.default.createDirectory(at: state, withIntermediateDirectories: true) + try Data("rows".utf8).write(to: state.appending(path: "db.sqlite")) + let after = ResourceFingerprint.capture(path: state.path) + #expect(ResourceFingerprint.compare(after: after, before: before) == .changed(.inode)) + } + + @Test func missingThenPresentIsAppearedAndTheReverseIsDisappeared() throws { + let dir = try scratch() + let file = dir.appending(path: "later.sqlite") + let absent = ResourceFingerprint.capture(path: file.path) + #expect(absent.kind == .missing) + try Data("x".utf8).write(to: file) + let present = ResourceFingerprint.capture(path: file.path) + #expect(ResourceFingerprint.compare(after: present, before: absent) == .appeared) + #expect(ResourceFingerprint.compare(after: absent, before: present) == .disappeared) + #expect(ResourceFingerprint.compare(after: absent, before: absent) == .unchanged) + } + + @Test func aFileReplacedByADirectoryIsAKindChange() throws { + let dir = try scratch() + let target = dir.appending(path: "thing") + try Data("x".utf8).write(to: target) + let before = ResourceFingerprint.capture(path: target.path) + try FileManager.default.removeItem(at: target) + try FileManager.default.createDirectory(at: target, withIntermediateDirectories: true) + let after = ResourceFingerprint.capture(path: target.path) + #expect(ResourceFingerprint.compare(after: after, before: before) == .changed(.kind)) + } + + /** Retargeting the link changes identity; editing what it points at does + not, because the walk never follows it. */ + @Test func symlinksAreNotFollowed() throws { + let dir = try scratch() + let a = dir.appending(path: "a") + let b = dir.appending(path: "b") + try Data("one".utf8).write(to: a) + try Data("two".utf8).write(to: b) + let link = dir.appending(path: "link") + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: a) + let before = ResourceFingerprint.capture(path: link.path) + #expect(before.kind == .symlink) + try Data("one-edited".utf8).write(to: a) + #expect(ResourceFingerprint.compare(after: ResourceFingerprint.capture(path: link.path), before: before) == .unchanged) + try FileManager.default.removeItem(at: link) + try FileManager.default.createSymbolicLink(at: link, withDestinationURL: b) + #expect(ResourceFingerprint.compare(after: ResourceFingerprint.capture(path: link.path), before: before) != .unchanged) + } + + /** Above the cap the digest is head, tail, size, and mtime, so a tail edit + is caught and a middle-only rewrite that preserves all four is not. The + limit is asserted rather than left to prose. */ + @Test func aLargeFileIsSampledAndSaysSo() throws { + let dir = try scratch() + let file = dir.appending(path: "big.sqlite") + let size = ResourceFingerprint.fileByteCap + 4096 + var bytes = Data(repeating: 0x41, count: size) + try bytes.write(to: file) + let before = ResourceFingerprint.capture(path: file.path) + #expect(before.exact == false) + + bytes[size - 1] = 0x42 + try bytes.write(to: file) + var attributes = try FileManager.default.attributesOfItem(atPath: file.path) + let stamp = try #require(attributes[.modificationDate] as? Date) + #expect(ResourceFingerprint.compare(after: ResourceFingerprint.capture(path: file.path), before: before) != .unchanged) + + /** The documented blind spot: same size, same mtime, same head and tail. */ + var middle = Data(repeating: 0x41, count: size) + middle[size / 2] = 0x43 + middle[size - 1] = 0x42 + try middle.write(to: file) + attributes[.modificationDate] = stamp + try FileManager.default.setAttributes([.modificationDate: stamp], ofItemAtPath: file.path) + let sampledAfter = ResourceFingerprint.capture(path: file.path) + let tailEdited = ResourceFingerprint.capture(path: file.path) + #expect(sampledAfter.digest == tailEdited.digest) + #expect(sampledAfter.exact == false) + } +} diff --git a/docs/cli-contract.md b/docs/cli-contract.md index efbe1ee..fe6e19f 100644 --- a/docs/cli-contract.md +++ b/docs/cli-contract.md @@ -6,7 +6,7 @@ The JSON surface agents depend on. Every schema here is generated from the Codab Success: the command's result object on stdout. Failure with `--json`: `{"ok": false, "error": {"code", "message", "hint"}}` on stdout; `hint` is the literal remediation command when one exists. -Stable `error.code` values: `already-exists`, `config-invalid`, `daemon-unreachable`, `internal-error`, `not-found`, `not-trusted`, `port-drift`, `port-held`, `resource-locked`, `spawn-failed`, `usage`, `version-mismatch`. (Grows append-only.) +Stable `error.code` values: `already-exists`, `config-invalid`, `daemon-unreachable`, `internal-error`, `not-found`, `not-trusted`, `port-drift`, `port-held`, `resource-locked`, `resource-mutated`, `spawn-failed`, `usage`, `version-mismatch`. (Grows append-only.) Exit codes: 0 ok · 1 operation failed (crash, timeout, conflict) · 2 usage · 3 daemon unreachable · 4 named server not found. Unnamed `status` in an unconfigured project exits 0 with `{"servers": []}`. @@ -76,6 +76,8 @@ Filled in per phase as each lands; golden tests reference the examples in this f - `devctl switch [--no-fetch] [--timeout 120]` → clean-tree guard (refuses dirty; never stashes), fetch, group down, `git switch` (remote-tracking fallback), then the project's `lifecycle.switch` playbook (argv arrays run sequentially from the project root; failures stop with `devctl up` as the resume hint), then group up. Playbooks live in devservers.json `lifecycle` and are agent-configurable. - Config extras: project-level `icon` (project-relative path, per-server override) feeds Spotlight thumbnails; every server and head is indexed in Spotlight as ` · ` with subtitle `devctl · ` (best-effort; not a Top Hit launcher); `heads` and pins surface in the menu bar app. - `devctl lock [--no-pause] [--acquire-timeout 300] [--timeout 120] -- ` → runs the command holding a project resource exclusively. By default the daemon pauses servers that declare the resource in their `locks` (devservers.json) and re-ensures them on release (even on command failure). `--no-pause` takes the mutex without stopping declarers (for harnesses that reuse the live server). `ensure`/`start` of a declaring server is refused (`resource-locked`, naming the holder pid) while a live holder owns it, regardless of `--no-pause`. Locks are path-scoped (`canonicalPath::resource`); they do not pause other checkouts. Locks persist across a daemon crash: a dead holder auto-releases and resumes the paused set; a still-live holder keeps them paused so the harness stays exclusive. Exit status is the command's. The `--` is required and devctl's own options go before it; everything after `--` is captured verbatim, so a nested `--`, a dash option, and an empty string all reach the command untouched. A missing terminator or an unknown option is rejected by the parser at exit 64 rather than being passed through. A contended acquire writes the holder's pid, how long it has been running, and what it paused or left running to stderr, then repeats a still-waiting line every 15s, so a wait is never silent; `--acquire-timeout 0` makes exactly one attempt and fails immediately with `resource-locked`. All of lock's own output is stderr: stdout belongs to the guarded command. + + A `locks` entry is written either as a bare name (`"d1"`) or as an object naming where the resource's state lives (`{"name": "d1", "path": ".wrangler/state/v3/d1"}`); both forms parse and the bare form re-encodes bare. With a path declared, `lock` fingerprints that state before and after the command (device and inode, size, and a SHA-256 over a bounded manifest) and reports a change: under the default paused mode it is a note on stderr, and under `--no-pause` with a declaring server still running it is a `resource-mutated` failure, because that server holds the old state open and can write its cached pages back over the change. Two servers declaring one resource with different paths is a config error rather than a guess. What the check cannot catch: it flags the risk window, not the damage, since the flush that corrupts can land after the command exits; it cannot see state outside the declared path (a sibling `-wal` file when `path` names only the `.sqlite`), divergence that never reaches disk, or a change that reverts to byte-identical state inside the window; above 8 MiB a file is sampled head and tail, so a middle-only rewrite preserving size and mtime is missed; and it never names which process wrote. - `devctl context`: the harness-agnostic session context: a fenced `` plain-text block (server phases, effective URLs, log paths, latent/rebound port-conflict warnings, the ensure/wait/why/logs/lock cheat-sheet) for the cwd's project. Linked worktrees get a banner naming the preferred host. Silent (exit 0) when the project is unregistered or untrusted or the daemon is down; never bootstraps; never contains raw log lines or command strings. - `devctl daemon status --json` → `{daemon?, launchd, reachable}`. `reachable` is whether the daemon answered over the socket, and it is the field to branch on: `launchd` reporting `running` only means a job is loaded, so a loaded-but-not-listening daemon prints a reassuring launchd line with `reachable: false`. `daemon` is present only when reachable. Exit stays 0 either way, because the launchd half is still a useful answer. - `devctl daemon install|uninstall [--purge]|start|stop|restart|status`: launchd lifecycle. `stop` drains and writes a deliberate-stop marker that auto-bootstrap honors; `restart` and `install` (upgrade) both capture running servers, bounce the daemon, and re-ensure them by name ("servers bounce, then come back"). The new daemon finishes `recoverAtStartup` before accepting socket clients, so that re-ensure cannot race a half-finished restore. `install` also stages-and-renames the daemon binary and captures the login-shell PATH into the agent plist. Reboot recovery: the LaunchAgent runs at load; starting a server records resume-on-boot; a machine shutdown drains without clearing it; `recoverAtStartup` resolves specs through the merged config+registry view (so committed `devservers.json` servers come back, not only ad-hoc `register` entries) and restores those servers one at a time so sibling port claims observe each other. A deliberate `devctl stop`/`down` clears the intent. Renamed or deleted servers leave orphan state rows that recover drops. diff --git a/scripts/smoke.sh b/scripts/smoke.sh index 401887d..367907a 100755 --- a/scripts/smoke.sh +++ b/scripts/smoke.sh @@ -376,11 +376,16 @@ pass "lock options before -- do not reach the guarded command" # A contended acquire has to name the holder rather than sit silent, and the # fail-fast form must return at once instead of waiting out the budget. -"$DEVCTL" lock data -- sh -c 'sleep 4' >/dev/null 2>&1 & +# The guarded command marks the file once it is actually running, so the checks +# below synchronize on the hold rather than racing a sleep. +rm -f "$WORK/held" +"$DEVCTL" lock data -- sh -c "touch '$WORK/held'; sleep 6" >/dev/null 2>&1 & HOLDER_JOB=$! -for _ in $(seq 1 60); do - if ! "$DEVCTL" lock data --acquire-timeout 0 -- true >/dev/null 2>&1; then break; fi +for _ in $(seq 1 100); do + [[ -f "$WORK/held" ]] && break + /bin/sleep 0.1 done +[[ -f "$WORK/held" ]] || fail "lock holder never started" FAST_START=$SECONDS set +e "$DEVCTL" lock data --acquire-timeout 0 --json -- true > "$WORK/lockfast.json" 2>/dev/null @@ -399,6 +404,42 @@ wait $HOLDER_JOB 2>/dev/null || true grep -qE "is held by pid [0-9]+" "$WORK/contended.err" || fail "contended lock waited silently: $(cat "$WORK/contended.err")" grep -q "waiting up to" "$WORK/contended.err" || fail "contended lock did not say the wait is bounded" pass "contended lock names the holder and bounds the wait" + +# The silent-clobber incident: a command that changes the locked state while a +# declaring server is still up cannot be distinguished from a clean run. Declare +# where the state lives (the object form of `locks`, alongside the bare string +# form asserted above) and the change is reported. +mkdir -p "$PROJECT3/state" +echo v1 > "$PROJECT3/state/db.sqlite" +/usr/bin/python3 - "$PROJECT3/devservers.json" <<'PY' +import json, sys +p = sys.argv[1] +cfg = json.load(open(p)) +cfg["servers"]["db"]["locks"] = [{"name": "data", "path": "state"}] +json.dump(cfg, open(p, "w")) +PY +"$DEVCTL" up --timeout 15 --json > /dev/null || fail "up before identity checks" + +# Paused mode: the change is the point, so it is a note on stderr and exit 0. +"$DEVCTL" lock data -- sh -c 'echo v2 > state/db.sqlite' 2>"$WORK/note.err" >/dev/null || fail "paused-mode lock failed" +grep -qE "note: 'data' state at .* changed" "$WORK/note.err" || fail "paused-mode change was not noted: $(cat "$WORK/note.err")" +pass "a change under a paused lock is reported as a note" + +# --no-pause with a live declarer: the server holds the old state open, so this +# is a loud failure rather than a silent success. +set +e +"$DEVCTL" lock data --no-pause --json -- sh -c 'rm -rf state && mkdir state && echo v3 > state/db.sqlite' > "$WORK/mutated.json" 2>/dev/null +MUTATED_EXIT=$? +set -e +[[ "$MUTATED_EXIT" -ne 0 ]] || fail "--no-pause accepted a command that replaced the locked state" +/usr/bin/python3 -c "import json;d=json.load(open('$WORK/mutated.json'));assert d['error']['code']=='resource-mutated', d; assert d['error']['hint'].startswith('devctl stop db'), d" || fail "resource-mutated envelope wrong: $(cat "$WORK/mutated.json")" +pass "--no-pause over changed state fails loudly with resource-mutated" + +# And an untouched resource stays quiet, so the check cannot fire on everything. +"$DEVCTL" lock data --no-pause -- true 2>"$WORK/quiet.err" >/dev/null || fail "--no-pause over untouched state failed" +[[ ! -s "$WORK/quiet.err" ]] || fail "--no-pause over untouched state was noisy: $(cat "$WORK/quiet.err")" +pass "an untouched locked resource stays silent" +"$DEVCTL" down --json > /dev/null "$DEVCTL" down --json > /dev/null # Deep links: print URL + dispatch via x-url (no Launch Services). From 1c37e09e101b37cb3a62a6a4d7e11d3112619de9 Mon Sep 17 00:00:00 2001 From: Evan Jacobs Date: Fri, 7 Aug 2026 23:50:27 -0400 Subject: [PATCH 3/9] docs: bring the map and the design record up to what shipped The codebase map gained no entry for Config/ConfigProjection, EffectiveHost, LockResource, Resource/ResourceIdentity, or the new CLI test target, and the smoke gate's description listed none of the assertions added with them. The design record still said `register --write` did not ship and that nothing writes devservers.json back, and its config model documented neither `locks` nor the two shapes a head may take. README now says how a lost gitignored config is recovered instead of telling the reader to keep a copy off the machine. --- AGENTS.md | 5 +++-- README.md | 4 ++-- docs/design.md | 13 ++++++++----- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 516ad5b..ffc5b7f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,9 +9,10 @@ Identity and stack - Three products, one daemon: devctld owns all server processes; devctl (CLI) and devctl.app (SwiftUI MenuBarExtra) are thin clients over a unix socket (default ~/Library/Application Support/devctl/daemon.sock; DEVCTL_SOCKET overrides; /tmp fallback near the sun_path limit), NDJSON protocol. devctl daemon install/uninstall/start/stop/restart manage the LaunchAgent (dev.quantizor.devctl); tests and the smoke gate run devctld --foreground. Codebase map -- Sources/DevCtlKit: shared core, the unit-test target of record. Models.swift (specs, phases, ServerStatus), Wire.swift (JSONCoding, typed request/response/event frames, NDJSON framing, stable error codes), Client/DaemonClient.swift (blocking-POSIX socket actor used unchanged by CLI and app), Paths/Paths.swift (path constants, canonical project path, atomic write + defensive load with per-call unique temp names so two writers in one process cannot rename each other's temp away, portable SHA-256; agent.path holds login-shell PATH for the daemon), Setup/SetupPlanner.swift (first-run / upgrade decisions, harness offers, stage-and-rename binary install), Agent/ (AgentContext: the pure session-context renderer the hook injects, bad-state servers first with a devctl why recommendation and devctl's own stderr count, never raw child output; DiscoveryStanza), Net/LoopbackProbe.swift (dual-stack loopback listen probe shared by the daemon port pre-check and CLI doctor), Net/PortClaim.swift + PortMaterializer.swift (effectivePort claim: portSpan and named ports; env injection and URL rewrite), Launchd/ (LaunchdAdmin: dual install path; SMAppService via app deep link when /Applications/devctl.app exists, else legacy home LaunchAgent + Application Support bin/devctld; --legacy forces the home path; DaemonRecoveryPolicy decides whether an unreachable daemon is auto-restarted; AgentRebindPolicy + agent.rebind settle the ad-hoc CDHash window on DMG replace), DeepLink/ (parse/serialize + DeepLinkRunner + notification action map), Log/DevCtlLog.swift (OSLog facade with a recording backend for tests). +- Sources/DevCtlKit: shared core, the unit-test target of record. Models.swift (specs, phases, ServerStatus), Wire.swift (JSONCoding, typed request/response/event frames, NDJSON framing, stable error codes), Client/DaemonClient.swift (blocking-POSIX socket actor used unchanged by CLI and app), Paths/Paths.swift (path constants, canonical project path, atomic write + defensive load with per-call unique temp names so two writers in one process cannot rename each other's temp away, portable SHA-256; agent.path holds login-shell PATH for the daemon), Setup/SetupPlanner.swift (first-run / upgrade decisions, harness offers, stage-and-rename binary install), Agent/ (AgentContext: the pure session-context renderer the hook injects, bad-state servers first with a devctl why recommendation and devctl's own stderr count, never raw child output; DiscoveryStanza), Net/LoopbackProbe.swift (dual-stack loopback listen probe shared by the daemon port pre-check and CLI doctor), Net/PortClaim.swift + PortMaterializer.swift (effectivePort claim: portSpan and named ports; env injection and URL rewrite, including resolving a root-relative head or healthcheck url against the server's own base), Config/ (ProjectConfig loader and validator; ConfigProjection projects merged specs back down to devservers.json, dropping everything the machine derived, for `config init`; EffectiveHost is the one home for the host a spawn will use, read by both prepareSpawn and config check; LocalOverlay; LockResource reads locks declarations and resolves a resource's state path), Resource/ResourceIdentity.swift (bounded fingerprint of a lock resource's state so `lock` can report a change made under a live holder), Launchd/ (LaunchdAdmin: dual install path; SMAppService via app deep link when /Applications/devctl.app exists, else legacy home LaunchAgent + Application Support bin/devctld; --legacy forces the home path; DaemonRecoveryPolicy decides whether an unreachable daemon is auto-restarted; AgentRebindPolicy + agent.rebind settle the ad-hoc CDHash window on DMG replace), DeepLink/ (parse/serialize + DeepLinkRunner + notification action map), Log/DevCtlLog.swift (OSLog facade with a recording backend for tests). - Sources/DevCtlDaemonCore: daemon logic as a library. Supervisor/ (ServerSupervisor actor per server: spawn, spool capture, health-gated phase machine, ensure/wait, group + descendant teardown with ProcessIdentity start-time revalidation; the ProcessLauncher seam; ProcessTree QA1123 sysctl sweep), Health/HealthProber.swift (EffectiveHealthcheck resolution, the HealthProber seam with ephemeral URLSession HTTP probes + BSD TCP, and PortGuard's lsof diagnostics, which live in that same file), Registry/ (owner of registry.json and state.json), Control/ (Router method dispatch + port pre-check + persisted resource locks with daemon-owned pause/resume and dead-holder auto-release + NWListener ControlServer with stateUpdateHandlers). - Sources/devctld: thin main; identical behavior under launchd and --foreground (tests and the smoke gate use foreground). Applies agent.path into process env before spawn. +- Tests: DevCtlKitTests is the unit-test target of record and holds the schema goldens; DevCtlDaemonCoreTests drives a real Router over temp paths; DevCtlCLITests covers CLI behavior with a contract and no other way to exercise it (argument parsing, the lock notices and identity verdict), importing the executable target with @testable. - Sources/devctl: CLI (swift-argument-parser). Two files only: HookSupport.swift (HookContext, the thin socket fetch over DevCtlKit's AgentContext renderer, + HarnessAdapter registry; adding a harness: CONTRIBUTING.md) and CLI.swift, which holds every command as a struct, including Switch (branch switching + lifecycle playbooks), Lock (run-under-resource-lock), Doctor (health report; owns the cross-project port-collision and squatter findings), and Link / x-url (deep links). CLI.swift is past the size where splitting is worth asking about. - Sources/DevCtlApp: menu bar app (DaemonModel 2s-polling model + crash notifications with Open/Why actions; AgentService wraps SMAppService.agent for Login Items registration, escalating to unregister + register when the agent reads `enabled` but the socket stays silent (a bootout or replaced bundle leaves the registration intact with nothing loaded) and reporting `requiresApproval` as its own failure instead of retrying; unregister records the stop intent so the recovery poll does not undo it; an unreachable daemon self-recovers via AgentService under DaemonRecoveryPolicy, falling back to legacy LaunchdAdmin only when the bundle carries no agent plist; DaemonDownRow offers Start, or Open Login Items while approval is pending, since a deliberate `daemon stop` stands down auto recovery; PresenceLabel is AppKit-drawn colored tally dots only with renderingMode(.original); popover autogrows to a cap; nested head rows with UserDefaults-persisted pins; DashboardView logs/timeline/config tabs; SpotlightIndexer named Core Spotlight index; AppDeepLink handles `devctl://` including `daemon/ensure` and `daemon/unregister`; SetupPanel first-run / upgrade installer from bundle Resources). Pure DaemonClient consumer. - Sources/fixture-server: test double dev server (heartbeat printer; TCP-listen, timed-exit, grandchild, ignore-sigterm, binary, flood modes; see its header comment). @@ -19,7 +20,7 @@ Codebase map Commands - make build: swift build -c release (all products) - make test: swift test; budget under 30s, the run prints the live timing -- scripts/smoke.sh: the end-to-end gate. Debug-builds, boots a real devctld on a temp socket, then asserts register/start/status, spool capture, health/ensure/wait, port conflicts, marks/events/why, resource locks (pause + refused ensure + resume), whole-group death on stop, child survival across a daemon kill, `link`/`x-url` deep-link dispatch, and that the assembled app declares `CFBundleURLSchemes=devctl`, ships AppIcon + CLI + daemon in Resources, and ships Helpers/devctld plus the in-bundle LaunchAgents BundleProgram plist. Run it after touching the supervisor, wire protocol, CLI, or deep links. +- scripts/smoke.sh: the end-to-end gate. Debug-builds, boots a real devctld on a temp socket, then asserts register/start/status, spool capture, health/ensure/wait, port conflicts, marks/events/why, config recovery (`config init` round-tripping through `config check`, its refusal to clobber, `register --write`), relative-head resolution and its `config check` rejection, resource locks (pause + refused ensure + resume, option parsing, a contended acquire naming the holder, and the identity check firing under `--no-pause` while staying silent otherwise), whole-group death on stop, child survival across a daemon kill, `link`/`x-url` deep-link dispatch, and that the assembled app declares `CFBundleURLSchemes=devctl`, ships AppIcon + CLI + daemon in Resources, and ships Helpers/devctld plus the in-bundle LaunchAgents BundleProgram plist. Run it after touching the supervisor, wire protocol, CLI, or deep links. - scripts/smoke-deeplink.sh: Launch Services E2E for `devctl://` (warm + cold `open`). Requires a GUI session; run before merging URL-scheme work. OSLog scrape is strict on a tty (`DEVCTL_OSLOG_STRICT=1` forces it). - scripts/smoke-launchd.sh: the REAL LaunchAgent lifecycle via `daemon install --legacy` (install, restart bounce + re-ensure, install-upgrade bounce + re-ensure, deliberate-stop intent, auto-bootstrap resurrection, uninstall). Mutates the user launchd domain; refuses to run if a home plist or bootstrapped job already exists; leaves nothing behind. SMAppService is exercised by installing from the app on a GUI session. - make app: assembles fat devctl.app via scripts/make-app-bundle.sh (CLI in Contents/Resources; signed Helpers/devctld + Contents/Library/LaunchAgents BundleProgram plist for SMAppService; AppIcon.icns; ad-hoc signed; SIGN_IDENTITY upgrades; declares the `devctl://` URL scheme). make dmg: UDZO image via scripts/make-dmg.sh, holding the app alone (no /Applications symlink: the app installs itself after an in-app confirm) over a background rendered by scripts/make-dmg-background.swift that says to double-click. Finder window layout needs a GUI session and a volume name that is not already mounted; on a headless runner that pass is skipped and the image still ships. scripts/notarize.sh: notarytool + staple. make install: CLI + daemon to ~/.local/bin, app to /Applications, daemon install. diff --git a/README.md b/README.md index 9f9024a..cc49e09 100644 --- a/README.md +++ b/README.md @@ -30,12 +30,12 @@ Name each server after the project (`myproj`, not a generic `web`) so it is easy Or write a `devservers.json` at the project root (multiple servers, dependencies, healthchecks, `*.localhost` host signatures, multi-headed proxies, lifecycle playbooks); `devctl up` brings the whole project up in dependency order. `devctl config check` validates the file against the daemon's own validator, the schema is in [docs/design.md](./docs/design.md), and the full CLI contract lives in [docs/cli-contract.md](./docs/cli-contract.md). -Commit that file where the whole team runs the same servers. Keep it gitignored and per-machine where the repository would rather not carry it: a shared checkout, a repository whose own docs should name no personal tooling, or a project where each person's ports and heads differ. Runtime behavior is identical either way; the difference is whether a fresh clone arrives with one. Nothing regenerates a gitignored file today, so keep a copy of it off the machine. +Commit that file where the whole team runs the same servers. Keep it gitignored and per-machine where the repository would rather not carry it: a shared checkout, a repository whose own docs should name no personal tooling, or a project where each person's ports and heads differ. Runtime behavior is identical either way; the difference is whether a fresh clone arrives with one. `devctl config init` writes the file back from what the daemon already knows, so a gitignored one that goes missing can be recovered. ## The parts - `devctld`: the daemon. Spool-file output capture (children survive daemon restarts without SIGPIPE), process-group plus descendant-sweep teardown, health-gated phases, crash forensics, structured logs with correlation marks, a unified event feed. -- `devctl`: the CLI. `ensure`, `wait`, `up`/`down`, `logs --since-mark`, `mark`, `events`, `why`, `open`, `switch`, `lock` (pause servers sharing a resource while a test harness runs), `doctor`, and launchd management. Agents are the first-class consumer. +- `devctl`: the CLI. `ensure`, `wait`, `up`/`down`, `logs --since-mark`, `mark`, `events`, `why`, `open`, `switch`, `lock` (pause servers sharing a resource while a test harness runs, and report when a command changed that resource while a server still held it open), `config init`, `doctor`, and launchd management. Agents are the first-class consumer. - `devctl.app`: the menu bar. Presence dots with counts, per-project rows with click-to-open heads (pinnable), crash notifications, a dashboard with live logs, an event timeline, and a validating config editor. Every server and head is Spotlight-searchable. ## Building diff --git a/docs/design.md b/docs/design.md index 054a69d..435879f 100644 --- a/docs/design.md +++ b/docs/design.md @@ -1,13 +1,14 @@ > Ratified plan snapshot (2026-07-18), kept as the design rationale record. The -> living documents are CLAUDE.md (map + invariants) and docs/cli-contract.md +> living documents are AGENTS.md (map + invariants) and docs/cli-contract.md > (the JSON surface). Known deviations from this plan, chosen during the build: > push subscriptions were replaced by incremental polling everywhere (CLI > --follow and the app; restart-safe, no reconnect machinery), logs.follow / > status.subscribe never shipped as wire methods, and the crash verb landed as > forensics inside `status` plus `devctl why` rather than `devctl crash`. The -> `register --write` back-to-file option and the automatic CLAUDE.md stanza -> offer did not ship (backlogged); resource locks (`devctl lock`) shipped -> beyond the plan. +> automatic agent-file stanza offer did not ship (backlogged). Beyond the plan: +> resource locks (`devctl lock`), and `devctl config init` plus +> `register --write`, which write devservers.json back from what the daemon +> knows. # devctl: macOS menu bar command center for dev servers @@ -73,11 +74,13 @@ Swift 6.3 (6.3.3 latest), Swift 6 language mode with strict concurrency. Depende - `command` is an argv array (no shell); `"shell": true` escape hatch runs via `/bin/zsh -lc` for nvm/mise-style setups, tradeoff documented. `cwd` is relative to project root (monorepo packages need it). - `shell: true` joins the argv array with single spaces and hands the result to `zsh`, so the two spawn modes are not equivalent: with the shell off, `["node", "my script.js"]` passes one argument containing a space, and with it on the shell re-splits that into two words. Quoting each element instead would break the other use of the escape hatch, where operators are written as separate elements (`["npm run dev", "&&", "echo done"]`). The rule is therefore: under `shell: true`, write the command as a single string element and quote inside it exactly as the shell requires. Multi-element argv is only safe when no element contains a space, glob, or quote. +- `locks` names the mutable resources a server holds while running, either as a bare name (`"d1"`) or as an object that also says where the state lives (`{"name": "d1", "path": ".wrangler/state/v3/d1"}`). Both forms parse and the bare form re-encodes bare. The path is what lets `devctl lock` fingerprint the state around a guarded command and report a change made while a declaring server was still up; two servers naming different paths for one resource is a config error rather than a guess. +- `heads` and `healthcheck.url` take either an absolute URL or a root-relative path (`/admin`), which resolves against the server's own effective base so it follows a rebind or a worktree host swap. Anything else, and a relative value on a server declaring neither `port` nor `url`, is a config-check error: it would otherwise materialize to `//:port/path`, which reads as a URL everywhere and works nowhere. - Host signature (Evan's isolation requirement): project-level `host` defaults to `.localhost`; per-server subdomain overrides allowed (`api.myproj.localhost`). Each server's `url` derives as `http://:/` unless set explicitly. Port ownership is enforced at start, not at registration (see checkout coexistence below). Unique origins keep browser cookies/storage/service workers isolated per project, so a bare `localhost` or `127.0.0.1` host (which collapses every project onto one origin) draws a config-check warning pointing at `.localhost`, never a hard error. Resolution caveat handled: browsers resolve `*.localhost` to loopback themselves, CLI tools and the system resolver do not reliably, so daemon healthchecks connect to 127.0.0.1 with the configured `Host` header, and the agent cheat-sheet says the same for curl. Backlog: devctl-managed reverse proxy on :80/:443 for port-free URLs. - Checkout coexistence: a git worktree is already a distinct project (distinct realpath, registry key, serverID, state, logs). Sibling checkouts share committed `devservers.json`, so only the scarce port collides. Before every spawn the daemon resolves an `effectivePort` (`ensure --port` > `devctl.local.json` overlay > persisted bound port from a prior sibling rebind > committed `port`) and a `PortClaim`: the primary plus optional `portSpan` (consecutive block for apps that derive children from one env) and/or named `ports` (relative `offset` that move with rebind, or absolute `port` that stay machine-singleton and `port-held` on collision). It injects `portEnv` (default `PORT`) and each named secondary's `env`, substitutes `{port}` / `{host}` in argv / url / heads / healthcheck, and advertises the rewritten URL on status (`ports` map when named secondaries exist). When any claimed relative port is held by a managed sibling (same `git` common-dir), the daemon auto-rebinds to a free claim block, persists the primary assignment, and still succeeds; unrelated projects and unmanaged squatters keep loud `port-held`. Linked worktrees also derive an ephemeral host `worktree-