Conversation
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: 8e3b76f
Top 3 slowest tests (at least 2 minutes):
|
anton-107
added a commit
that referenced
this pull request
Aug 27, 2026
…dover Fixes the two defects found verifying #6387 on real compute (DECO-28187). 1. The linger never released early. `detached_descendants()` asked `ps` for this process's children, but `ps` is itself one of them and `subprocess.run` leaves it in the notebook's process group, so it passed the "outside the server's group" filter on every poll. The survivor list was never empty: `--keep-detached-for` held the run - and suppressed cluster autotermination - for its full duration even with zero detached work, and "No detached processes left, releasing the run" was unreachable. It now reads `/proc` directly, mirroring `detachedDescendants` in `internal/server/descendants.go`, which counted correctly for exactly this reason. 2. A server handover marked the previous run FAILED. A new session's bootstrap terminates a server already running on the cluster, which a reconnect asking for a different `--keep-detached-for` now reaches on a normal path; the previous notebook saw -15 and raised, so its run ended INTERNAL_ERROR/FAILED. A SIGTERM exit is now treated as a handover rather than a failure. The comment beside `needNewServer` claimed the displaced server "idles out via shutdownDelay" - it is killed; corrected. Also documents what the verification measured about the feature's real ceiling: the work lives at most the linger plus the cluster's autotermination window, reconnecting after the linger leaves survivors with EPERM on every workspace path, and a reconnect that omits the flag inherits the linger of the server it reuses. Verified locally under a PR_SET_CHILD_SUBREAPER parent with a real `setsid` child: the old code reports a phantom survivor with a fresh pid each call and never empties, the new code reports the one real detached process and empties as soon as it exits. `./task test-exp-ssh` (327 unit + 7 acceptance), `./task lint-q`, ruff format and the whitespace check pass. Co-authored-by: Isaac <no-reply@databricks.com>
Today the SSH tunnel destroys work that was deliberately detached. When the server
exits on its idle timer, the bootstrap notebook sweeps every process it parents with
`pkill -P`, and because it sets PR_SET_CHILD_SUBREAPER it parents every orphan in the
session - which is exactly what tmux, setsid and nohup make of themselves.
Scoping that sweep to the server's own process group is not enough on its own, and
that is the part measured on real compute (DECO-28187): survivors do live through the
teardown, but the notebook is the WSFS-registered process, and WSFS authorises an I/O
by walking the live process tree for a registered ancestor. The moment the notebook
returns, the survivors reparent to PID 1 and every /Workspace and /Volumes call fails
with EPERM. That turns a visible failure into an invisible one.
So the two halves ship together, behind one flag:
databricks ssh connect --cluster=<id> --keep-detached-for=<duration>
With it set, teardown terminates only the server's process group and the notebook then
holds the job run open while any detached process is still alive, so the anchor stays
in place. With it unset - the default - the notebook sweeps its children exactly as
before, and the server logs a warning naming the processes it is about to destroy, so
the original complaint (work vanished with no explanation) becomes actionable even for
users who never set the flag.
A duration rather than a boolean, because what the feature spends is time on a
cluster: a held-open run suppresses autotermination. It is capped at the run's own
timeout, rejected for serverless (the container teardown takes survivors regardless),
and plumbed like --usage-policy-id: notebook base parameter, persisted in
metadata.json, and a reconnect asking for a different value starts a new server.
Telemetry: the connect event records whether the flag was asked for. Whether the
session actually left detached work behind is only visible on the compute at teardown,
so the server emits a new SshTunnelTeardownEvent for it - separate from SshTunnelEvent
so it cannot be counted as a connection. That number is what tells us how often we
destroy work today, and so whether the default should ever flip.
Also narrows the startup sweep, which matched any command line containing
`databricks_cli` and would otherwise let a new session destroy a previous session's
detached work.
Co-authored-by: Isaac <no-reply@databricks.com>
…dover Fixes the two defects found verifying #6387 on real compute (DECO-28187). 1. The linger never released early. `detached_descendants()` asked `ps` for this process's children, but `ps` is itself one of them and `subprocess.run` leaves it in the notebook's process group, so it passed the "outside the server's group" filter on every poll. The survivor list was never empty: `--keep-detached-for` held the run - and suppressed cluster autotermination - for its full duration even with zero detached work, and "No detached processes left, releasing the run" was unreachable. It now reads `/proc` directly, mirroring `detachedDescendants` in `internal/server/descendants.go`, which counted correctly for exactly this reason. 2. A server handover marked the previous run FAILED. A new session's bootstrap terminates a server already running on the cluster, which a reconnect asking for a different `--keep-detached-for` now reaches on a normal path; the previous notebook saw -15 and raised, so its run ended INTERNAL_ERROR/FAILED. A SIGTERM exit is now treated as a handover rather than a failure. The comment beside `needNewServer` claimed the displaced server "idles out via shutdownDelay" - it is killed; corrected. Also documents what the verification measured about the feature's real ceiling: the work lives at most the linger plus the cluster's autotermination window, reconnecting after the linger leaves survivors with EPERM on every workspace path, and a reconnect that omits the flag inherits the linger of the server it reuses. Verified locally under a PR_SET_CHILD_SUBREAPER parent with a real `setsid` child: the old code reports a phantom survivor with a fresh pid each call and never empties, the new code reports the one real detached process and empties as soon as it exits. `./task test-exp-ssh` (327 unit + 7 acceptance), `./task lint-q`, ruff format and the whitespace check pass. Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
force-pushed
the
deco-28187-implement-option-as-described-in-26
branch
from
September 16, 2026 19:10
c6560ba to
5df516e
Compare
Replaces --keep-detached-for=<duration> with --keep-detached-processes, a boolean bounded by the run's own timeout (--server-timeout, 24h by default). The duration was a second budget for the same run, measured from a different origin: the run timeout runs from run start, while the linger clock only started once the server exited. --keep-detached-for=24h under the default --server-timeout therefore validated but could never deliver 24h, and since Validate compared the two in isolation, the ceiling it accepted silently depended on how long the session had already run. Presence is the condition the feature wants - hold the run while detached work is alive - and --server-timeout already means "how long this run may live", so it can carry the cost control on its own. Dropped along with the duration: the negative-value check, the ceiling check, the seconds-based notebook widget, and duration matching on reconnect. keepDetachedMatches is now "asking for it needs a server that has it". The notebook's hold has no deadline any more, so its per-poll log line is throttled to one every five minutes while the set of survivors is unchanged, instead of every 15 seconds for as long as the work lives. Co-authored-by: Isaac <no-reply@databricks.com>
The mode is fixed when the server job is submitted, and for a host configured by `ssh setup` the submitting invocation is always the persisted ProxyCommand - the same reason --max-clients and --server-timeout are carried there. Without this, an `ssh <name>` session could never ask to keep its detached work: it either inherited the mode from a server someone had already started with `ssh connect --keep-detached-processes`, or started one that swept the work. Setup already validates the ClientOptions it serializes, so the dedicated-only check applies here too; setup requires a cluster, so it cannot trip. Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
changed the title
## Changes Add a non-reaping `waitid` check before releasing the SSH bootstrap run, plus six regression scenarios wired into the SSH Go test suite. ## Why Targets #6387. After SIGTERM, server-group helpers can still parent detached work that the notebook has not adopted yet. An empty survivor scan must not release the run while children remain. The existing SIGCHLD handler retains ownership of exit statuses, and polling/report throttling stay unchanged. ## Tests - Regression cases fail on the unpatched parent (`b3c4b6c3c`) and pass with this fix. An isolated Linux process reproduction also verifies delayed adoption and exit-status preservation. - `./task fmt`, `./task checks`, `./task lint`, and `./task test-exp-ssh` pass. - `./task test` passes with `OMNIGENT` unset, cloud testing disabled, and an isolated `TMPDIR`; two unrelated Terraform acceptance timeouts pass on automatic retry. _This PR was written by Codex._ --- <!-- GITHUB_MCP_FOOTER: This attribution is automatically appended by GitHub MCP. --> _This PR was created with [GitHub MCP](http://go/mcps)._
rclarey
reviewed
Sep 18, 2026
Comment on lines
+151
to
+152
| // On serverless the container goes away with the run, so nothing survives the server | ||
| // however long the notebook holds the run open. |
Contributor
There was a problem hiding this comment.
I don't follow. You say the container goes away with the run, but doesn't that mean holding the run open keeps the container?
Also this seems like a completely different reason compared to the design doc for why --keep-detached-processes is only supported on dedicated clusters
Comment on lines
+14
to
+19
| module = ast.parse(source.read_text()) | ||
| module.body = [ | ||
| node | ||
| for node in module.body | ||
| if isinstance(node, ast.FunctionDef) and node.name in {"has_children", "wait_for_detached_descendants"} | ||
| ] |
Contributor
There was a problem hiding this comment.
This seems a bit hacky, can we export those functions and test them normally instead?
Comment on lines
+87
to
+88
| the cost is what matters; multi-day work still belongs in Jobs/DABs. Note also that | ||
| reconnecting starts a new run rather than rejoining the one being held open, so each session |
Contributor
There was a problem hiding this comment.
Why do we shutdown the server while detached processes are still running? Won't people want to reconnect to their tmux sessions they left running?
Contributor
Author
There was a problem hiding this comment.
--server-timeout remains to be an ultimate kill switch to keep runaway workloads at cap budget and it is a setting on the job level. This PR does not change that
anton-107
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Adds
--keep-detached-processestodatabricks ssh connectanddatabricks ssh setup(dedicated clusters, default off), which lets processes detached from the SSH session (tmux,setsid,nohup) outlive the tunnel:ssh-server-bootstrap.py): when the flag is set, terminate only the server's own process group - the server and itssshdchildren - and then hold the job run open while any detached process is still alive. Unset, the notebook sweeps its children exactly as before.internal/server/): the idle-timeout path returns instead of callingos.Exit(0), and before returning it looks for detached processes. With the flag unset it logs a WARN naming them (readable via/logs), and it emits a newSshTunnelTeardownEvent.metadata.json, so a reconnect that asks for the mode against a server started without it gets a new server. Same shape as--usage-policy-id. The flag is also carried in the persistedProxyCommand, like--max-clientsand--server-timeout, so a host configured throughssh setupcan ask for it - that ProxyCommand is the invocation that submits the run, and the mode is fixed at submission.databricks_cliand would otherwise let a new session destroy the previous session's detached work.Implements option A from DECO-28187.
Why
The tunnel currently destroys work users deliberately detached: on server exit the bootstrap notebook runs
pkill -Pon everything it parents, and since it setsPR_SET_CHILD_SUBREAPERit parents every orphan in the session - which is exactly whattmux,setsidandnohupproduce.Scoping that sweep to the server's process group is not enough on its own, and this is the part measured on real compute: survivors do live through teardown, but the notebook is the WSFS-registered process and WSFS authorizes an I/O by walking the live process tree for a registered ancestor. Once the notebook returns, survivors reparent to PID 1 and every
/Workspaceand/Volumescall fails withEPERM- a visible failure traded for an invisible one. So the narrowed teardown and the held-open run ship together behind one flag.Why a boolean, bounded by
--server-timeout. The hold ends when the last detached process exits; the only other bound is the run's own lifetime, which--server-timeoutalready sets (24h by default) and Jobs already enforces. An earlier revision of this PR took a duration instead (--keep-detached-for), which was a second budget for the same run measured from a different origin: the run timeout runs from run start, while the linger clock only started once the server exited.--keep-detached-for=24hunder the default--server-timeouttherefore validated but could never deliver 24h, and becauseValidatecompared the two in isolation, the ceiling it accepted silently depended on how long the session had already run. Presence is the condition the feature actually wants, and one knob for "how long this run may live" is enough. Dropped with the duration: the negative-value check, the ceiling check, the seconds-based notebook widget, and duration matching on reconnect.What the flag spends is time on a cluster: a
RUNNINGrun suppresses autotermination (measured). That is why it is off by default, and why it is rejected for serverless, where the container teardown takes survivors regardless. Because the hold now has no deadline, the notebook's "holding the run open" line is throttled to one every five minutes while the set of survivors is unchanged, instead of one per 15s poll for as long as the work lives.The teardown warning is useful on its own: it turns "my work vanished with no explanation" into an actionable message even for users who never set the flag.
SshTunnelTeardownEventis a new event rather than fields onSshTunnelEvent, so teardown rows can't be counted as connections by existingis_successqueries. It needs a matching lumberjack proto in universe (follow-up). Only the request is reported, not how long the hold lasted: the notebook outlives every Go process in the session, so no CLI process can observe when the hold ends - the run's own duration carries that.Tests
/proc), the teardown warning across flag/compute combinations, and the teardown telemetry asserted end-to-end through the upload path.ToProxyCommand, the submit-run base parameters,keepDetachedMatches, the connect event field, and thessh setuphost config.acceptance/sshconnect goldens carry the new base parameter, andacceptance/ssh/setuppins the flag in the ProxyCommandssh setupwrites../task lint,./task fmt,./task ws,go vet, ruff format/check on the bootstrap, and the fullexperimental/ssh+libs/telemetryunit suites pass.Live verification. The feature was verified end to end on two dedicated dogfood clusters (results, findings): detached work survives the tunnel, the run is held open, WSFS stays intact for the whole hold,
tmux attachworks on the next connection, and with the flag unset teardown is indistinguishable from the old build. Two defects came out of those runs and are fixed here:detached_descendants()askedpsfor this process's children, butpsis itself one of them andsubprocess.runleaves it in the notebook's process group, so it passed the "outside the server's group" filter on every poll. The survivor list was never empty, so the run - and the cluster - was held for the full duration even with zero detached work. It now reads/procdirectly, mirroringdetachedDescendantsininternal/server/descendants.go, which counted correctly for exactly this reason.FAILED. A new session's bootstrap terminates a server already running on the cluster, which a reconnect asking for a different mode reaches on a normal path; the previous notebook saw-15and raised. A SIGTERM exit is now a handover rather than a failure.That verification ran while the flag was still a duration. Everything it exercised - group-scoped teardown, the run held open, WSFS intact for the whole hold,
tmux attachon reconnect, teardown unchanged with the flag unset - is untouched by the rename; what changed is how the hold ends (the work exiting, or the run timing out, instead of a preset deadline). Not re-verified on live compute after the/procand handover fixes, or after the rename.This pull request and its description were written by Isaac.