Skip to content

fix: bind daemon registration writes to the acquired owner - #3125

Merged
thymikee merged 5 commits into
fix/daemon-termination-outcomesfrom
fix/daemon-registration-owner
Oct 3, 2026
Merged

thymikee merged 5 commits into
fix/daemon-termination-outcomesfrom
fix/daemon-registration-owner

Conversation

@thymikee

@thymikee thymikee commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

Summary

Daemon startup and shutdown now use one acquisition-bound registration owner. It acquires the shared lock at daemon.lock, clears the previous shutdown report, publishes metadata, and finishes by writing the report, retiring matching metadata, and releasing its acquisition.

publish and finish assert ownership immediately before mutations. Spent or displaced handles cannot overwrite a successor. Legacy lock files are retained and refused. Dedicated busy/unproven startup codes distinguish contention from startup failure.

14 files changed; 995 gross lines. Builds on #3124; part of #3116. Client retirement and startup adaptation follow in this stack. Deploy these coupled changes together; mixed legacy clients remain unsupported.

Validation

Tested 30a2f7c468:

  • AGENT_DEVICE_REQUIRE_LOOPBACK_TESTS=1 pnpm check:affected --base fix/daemon-termination-outcomes --run passed: 48 related files, 400 tests; all runnable checks completed.
  • Removing acquisition assertions fails the spent-owner regression. Suppressing warnings and delaying the shutdown watch stop each fail their regressions.
  • Runtime diagnostics preserve message, hint, details and existing decline fields.
  • Independent review confirmed acquisition and lifetime boundaries. GitHub CI and coverage remain authoritative on the new head.

@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.95 MB 4.95 MB +41 B
Package (unpacked) 4.94 MB 4.95 MB +41 B
Package (download) 1.48 MB 1.48 MB -16 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 25.8 ms 26.2 ms +0.4 ms
CLI --help 82.8 ms 81.8 ms -1.0 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All reported issues were addressed across 14 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread src/daemon-registration-owner.ts Outdated
Comment thread src/daemon/server/daemon-runtime-metadata-ownership.test.ts Outdated
@thymikee

thymikee commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

The daemon side of this PR looks right, but two defects need fixing before merge (reviewed at aea5196). The failing smoke check looks unrelated: the daemon started in 230ms and the run then failed on an iOS runner connect error (wait_runner_restart_exhausted), and 14 of 15 smoke tests passed. No conflicts.

First, daemon.lock is now a directory, but the client still reads and writes it as a file. In daemon-client-metadata.ts, readDaemonLockInfo hits EISDIR and returns null, so isDaemonLockHeldByAnotherDaemon is always false. Two clients that start at once now both spawn. The loser exits 75, and waitForDaemonStartup treats that as early_exit instead of waiting for the winner. Then cleanupFailedDaemonStartupMetadata can stop the winner if its daemon.json is not yet connectable, and the retries hit busy again. recoverDaemonLockHolder (line 243) reports "recovered" while the unlink on a directory is swallowed. The cleanup hint at line 304 tells users to run rm -f daemon.lock, which fails on a directory and skips daemon.reclaim.lock. Concurrent CLI calls on one state dir are a common pattern, and it worked before this PR. The rule: every client reader or writer of paths.lockPath must use the process-lock directory protocol and the busy exit code 75. A grep of lockPath in src/daemon-client lists the sites, including daemon-client-timeout.ts:192. Route them through inspectProcessLock and DAEMON_STARTUP_EXIT_CODES, and keep the legacy-file branch only for upgrading a stale legacy file. Please add a regression test where a second registration attempt exits busy and the client adopts the first daemon. I read this from the code and did not reproduce the race, and the winner-takeover step depends on timing.

Second, publish() in daemon-registration-owner.ts truncates daemon.log with publishFileSync, which writes a temp file and renames it over the log. The client opens the daemon's stdout and stderr on daemon.log in append mode (daemon-client-lifecycle.ts:681). The old code truncated the same inode. The rename gives the log a new inode, so the daemon keeps writing to the unlinked one. After publish, the port line, "Daemon error" lines, crash traces and console output never reach daemon.log, and the client's startup-failure tail misses them too. The log is not a published record, so please truncate it in place after assertHeld (for example fs.writeFileSync(logPath, '', { mode: 0o600 })) and keep the atomic publish for daemon.json only. Please add a test that opens an append fd on logPath before publish, writes after it, and checks the bytes land in daemon.log. That test should fail on this head. I did not check Windows, where renaming over an open file may also fail.

Would a smaller overall change be to move the lock layout and the client lock readers in one PR? I found nothing smaller on the daemon side, and the net -24 lines is good. But today the daemon and the client disagree on what daemon.lock is. Either migrate the client readers here, or confirm that the stack merges atomically. I did not check whether the follow-up client PR fixes the first issue.

I did not run tests. I could not read the smoke job's daemon.log or runner.log, so the runner connect classification rests on the log excerpt alone. Before merge, fix the log truncation and make the client lock readers use the directory protocol, or fold the client migration into this PR.

@thymikee
thymikee force-pushed the fix/daemon-registration-owner branch from aea5196 to 30a2f7c Compare October 2, 2026 23:27
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Two problems from the earlier review (aea5196) are still open at 30a2f7c, because the new commit is only a rebase. Smoke Tests fails on the cold launch deep link, which looks unrelated, and I know of no conflicts.

The daemon now takes daemon.lock as a process-lock directory (daemon-registration-owner.ts:50), but the client still reads it as a JSON file (daemon-client-metadata.ts:113). The read fails with EISDIR and returns null, so isDaemonLockHeldByAnotherDaemon is always false. removeDaemonLock and the timeout reset try to unlink a directory and swallow the error. When two CLI calls start at once, both spawn a daemon, the loser exits busy (75), and the client treats that as an early exit instead of adopting the winner. Cleanup can then stop the winner before its daemon.json is connectable. The hint that says to run rm -f daemon.lock also does not work on a directory. The rule: every client read or write of paths.lockPath must use the process-lock directory protocol (inspectProcessLock) and the busy exit code in DAEMON_STARTUP_EXIT_CODES. A legacy-file branch should stay only to upgrade a stale pre-PR file. git grep lockPath -- src/daemon-client lists the sites. Please add a regression where a second registration attempt exits busy and the client adopts the first daemon. Or fold the client migration from the stacked PR into this one.

publish() in daemon-registration-owner.ts:67 writes a temp file and renames it over daemon.log. The daemon's stdout and stderr are append fds on the old inode, opened at spawn in daemon-client-lifecycle.ts. After publish, the port line, error lines and crash traces go to an unlinked file, so they never reach daemon.log and the client's startup-failure tail misses them. After assertHeld(), please truncate the log in place (fs.writeFileSync(logPath, '', { mode: 0o600 }) or ftruncate) and keep the atomic publish for daemon.json only. A test should open an append fd on the log before publish, write after it, and assert the bytes are in daemon.log. That test should fail on this head.

I did not run tests or reproduce the concurrent-start race. The first finding is read from the code, and the winner takeover step depends on timing. I did not check Windows rename-over-open-file behavior, and I did not check whether the stacked client PR already migrates the lock readers.

The Smoke Tests failure is likely unrelated. Daemon startup took 38ms, then the iOS open request hung for 90s and the daemon was force killed. This PR does not touch the open, runner or app-launch route, and the same signature appears on #3130. I could not read the job's daemon.log or runner.log, so this rests on the log excerpt only. Before merge, the daemon.log rename must be fixed and the client lock readers must move to the process-lock directory protocol.

@thymikee
thymikee added this pull request to stack #3147 October 3, 2026 08:47
@thymikee
thymikee force-pushed the fix/daemon-registration-owner branch 2 times, most recently from 134521a to f55d235 Compare October 3, 2026 16:41
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The two cubic-dev-ai threads are fixed at f55d235, but one defect from the earlier review is still open: #3125 (comment)

The daemon now takes daemon.lock as a process-lock directory (daemon-registration-owner.ts:50). The client still reads it as JSON in daemon-client-metadata.ts:113. That read fails with EISDIR and returns null, so isDaemonLockHeldByAnotherDaemon is always false. removeDaemonLock (line 146) and the reset at daemon-client-lifecycle.ts:456 unlink a directory and swallow the error. The hint at line 305 still says rm -f and skips the reclaim guard. When two CLI calls start together, the losing daemon exits busy (75). The client reads that as early_exit instead of adopting the winner. cleanupFailedDaemonStartupMetadata can then stop the winner before its daemon.json is connectable. So concurrent calls on one state dir can spawn competing daemons, and stale-lock recovery reports success without removing anything. This worked before the PR. The rule: every client reader and writer of paths.lockPath goes through the process-lock directory protocol (inspectProcessLock from host-kit) and treats DAEMON_STARTUP_EXIT_CODES.busy as "adopt the winner". Keep a legacy-file branch only to upgrade a stale pre-PR file. git grep lockPath -- src/daemon-client lists every site. Please add a regression where a second registration attempt exits busy and the client adopts the first daemon. Or fold the stacked client migration into this PR.

The review at f55d235 did not run the tests, did not reproduce the race (it is read from the code), and did not check Windows behavior of ftruncate on an append fd that another process holds open. Both Smoke Tests jobs are still queued, and every smoke run goes through the daemon startup this diff changes, so a failure there needs its daemon.log read before anyone calls it unrelated. There are no conflicts. The next step before merge is the client lock migration above, with the busy-exit regression.

Resolve these two threads, which are fixed at f55d235: the scope for releaseRegistrationAfterFailure (#3125 (comment)) and the test match for stopMetadataLossWatch() and finishDaemonRegistration( (#3125 (comment)).

The daemon's stdout and stderr hold append descriptors on daemon.log, so an
atomic rename gave the log a new inode and later output went to the unlinked
file. Publication now truncates the existing inode. The held-lock provider
shutdown test now loses to a real registration instead of a removed helper.
@thymikee
thymikee force-pushed the fix/daemon-registration-owner branch from f55d235 to b5e49b3 Compare October 3, 2026 17:49
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The client side of this is in the stack above this PR, not here. #3129 and #3130 move every client reader and writer of daemon.lock to the process-lock protocol (inspectProcessLock). At the top of the stack no JSON read of the lock path is left under src/daemon-client. daemon-client-startup-race.test.ts covers a busy contender adopting the real winner. As the PR body says, the stack merges and deploys as one unit, with no release cut part-way through, so main never has the daemon and client disagreeing about daemon.lock. Folding the client migration into this PR would mean moving it across four PRs, so I kept it where it is.

The daemon.log truncation is fixed here: publish now empties the log in place, and a test holds an append fd across publish and checks that later writes reach the file.

@thymikee
thymikee merged commit 57b0ff1 into main Oct 3, 2026
19 checks passed
@thymikee
thymikee deleted the fix/daemon-registration-owner branch October 3, 2026 18:04
thymikee added a commit that referenced this pull request Oct 3, 2026
…token-attach-c41aff

* commit 'd396f3b509d9ed7cddaf170351ea6cf34e02ac16':
  fix(provider-webdriver): harden BrowserStack app references and endpoints (#3169)
  fix(android): back off a timed-out snapshot helper session and bound content re-captures (#3160)
  fix: centralize confirmed daemon retirement (#3126)
  fix: bind daemon registration writes to the acquired owner (#3125)
  fix: return confirmed daemon termination outcomes (#3124)
  refactor(move): share daemon registration and shutdown report modules (#3123)
  fix: preserve process lock exclusion across publication and reclaim (#3122)
  fix(cli): refuse a non-URL install-from-source source up front (#3166)
  fix(daemon): start a lease's TTL when its allocation completes (#3165)
  fix(android): fail doctor when adb is the Windows binary on a POSIX host (#3157)
  0.21.20
  0.21.19

# Conflicts:
#	src/daemon/server/daemon-runtime-metadata-ownership.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant