fix(sdk): don't SIGKILL a database dump copy after 30 seconds - #3643
Merged
Conversation
`SubContainer.exec` defaults `timeoutMs` to 30000 and enforces it with SIGKILL; `execFail` forwards the argument with no default, so any call site that omits it inherits 30 s. 1.5.2 split `pg_dump`-onto-the-target into a local dump plus a `cp` through the backup-fs FUSE, kept the explicit `null` on `pg_dump`, and gave the new `cp` none — so once a dump grew large enough to take more than thirty seconds to copy, every backup failed with `cp terminated with signal SIGKILL:`. Restore stages the dump off the target through the same kind of copy under the same cap, so such a database could not be restored at all. Every step of a dump or restore whose duration follows the data now opts out through a named `NO_TIMEOUT`: both copies, `pg_ctl` start and stop, the recursive `chown`s, `initdb`, the MySQL/MariaDB init, and the foreground `mysqld` MariaDB runs for the length of the dump. `pg_ctl` bounds itself — `-w` is its default and it gives up after `-t` seconds, 60 by default — so removing the SDK cap only moved that limit from 30 s to 60 s, out of reach of `readyTimeout`, the knob packages are told to raise. `readyTimeout` now supplies `-t`; its 60 s default is `pg_ctl`'s own, so nothing changes until it is raised. A timeout kill was reported as a bare signal, indistinguishable from an OOM kill, a cgroup kill, or an operator's `kill -9` — and `cp` writes nothing to stderr when killed, so the whole notification was `cp terminated with signal SIGKILL:`. `exec`'s result now carries `timedOutAfterMs` and the error names the limit that elapsed. The flag is decided in the exit handler rather than the timer, because the deadline can elapse after something else has already killed the process. Also corrects two TSDoc claims 1.5.2 falsified (the dump is staged in `/tmp`, not written straight to the target, and `dumpVolume` does not exist), and documents the `timeoutMs` argument in the packaging guide, where the 30 s default went unmentioned. Fixes #3636
dr-bonez
reviewed
Aug 11, 2026
A bare `null` in the third position gave no hint which knob it was
setting or what the value meant, and reaching `abort` in the fourth
meant supplying the third. Both move into the options object, so
`sub.execFail(cmd, { user: 'root' }, null)` becomes
`sub.execFail(cmd, { user: 'root', timeoutMs: null })` — which retires
the `NO_TIMEOUT` constant this branch had added to make the positional
form legible.
They move together because dropping only `timeoutMs` would leave
`abort` sliding into a position whose type it does not match. Passing
either positionally is now a compile error rather than a silent
misread.
`exec` forwards the remainder of its options to `cp.spawn`, so the two
are destructured out. That destructure also copies, where the existing
code deleted `user` and `cwd` off the caller's own object — with
`timeoutMs` on it, a reused options object would have silently dropped
back to the 30 s default on its second use.
Milliseconds are the assumed unit for a duration in JS, and the rest of this surface already names them without one — `readyTimeout`, `sigtermTimeout`, `gracePeriod`. `timeoutMs` becomes `timeout`, and the result field `timedOutAfterMs` becomes `timedOutAfter` for the same reason. The unit moves to the option's doc comment, and the error message still spells it out for whoever reads it. The long-standing `timeoutMs` parameters in container-runtime's System adapters are left alone; only the boundary where DockerProcedureContainer hands one to `subcontainer.exec` is mapped across.
dr-bonez
approved these changes
Aug 11, 2026
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
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.
Fixes #3636.
The bug
SubContainer.execdefaultstimeoutMsto 30 000 and enforces it withSIGKILL.execFaildeclarestimeoutMswith no default and forwards it, so any call site that omits the argument silently inherits 30 s.336eda75(1.5.2) split "pg_dumpwrites onto the backup target" into "pg_dumpwrites to/tmp, thencpthrough the FUSE". The explicitnullstayed onpg_dump; the newcp— the step whose duration is set by the size of the dump and the speed of the target — got none. Once a dump grew past a thirty-second copy, every backup failed:Restore stages the dump off the target through the same kind of copy under the same cap, so a database whose dump took longer than 30 s to copy could not be restored at all — discovered during recovery, when the backup is all the user has.
What changed
Every step of a dump or restore whose duration follows the data now opts out, through a named
NO_TIMEOUT(which also replaces the file's existing barenulls, so the opt-out is greppable): both copies,pg_ctlstart/stop, the recursivechowns over the data directory,initdb,mysql_install_db/mysqld --initialize-insecure, and the foregroundmysqldMariaDB runs for the length of the dump. The steps left at 30 s touch a fixed, tiny number of inodes (touch,mkdir -p,rm -f, single-filechown,createdb,psql ALTER USER) or are already bounded by their own poll loop (pg_isready,mysqladmin ping).readyTimeoutnow reachespg_ctl.pg_ctlbounds itself —-wis its default and it gives up after-tseconds, 60 by default — so lifting the SDK cap alone would only have moved the ceiling 30 s → 60 s, out of reach of the one knob packages are told to raise for slow clusters.readyTimeoutnow supplies-t. Its 60 000 ms default ispg_ctl's own default, so behaviour is unchanged until someone raises it.A timeout kill now says it timed out. The error was built from the signal alone, so the SDK's own timer was indistinguishable from an OOM kill, a cgroup kill, or an operator's
kill -9— andcpwrites nothing to stderr when killed, leaving the barecp terminated with signal SIGKILL:above as the entire notification.exec's result now carriestimedOutAfterMsand the message names the limit:The flag is decided in the exit handler, not the timer: the deadline can elapse after something else has already killed the process (the
'exit'event is delivered in the poll phase, after timers), and setting it from the timer reported an externalSIGTERMas an SDK timeout — the same misattribution inverted.timedOutAfterMsis set only when our timer fired and the process died ofSIGKILL.Docs. The packaging guide documented
exec/execFailwithout ever mentioning the 30 s default — the omission this issue is made of, and its example was agit clone. It now documents the third argument. Two TSDoc claims that 1.5.2 falsified are corrected: the dump is staged in/tmprather than "written directly to the backup target — no data duplication on disk", andwithMysqlDumpreferred to adumpVolumeoption that does not exist inMysqlDumpConfig.While in the file, the four hand-rolled structural
sub:parameter types inBackups.tsare replaced with the realSubContainer<M>— one of them had to be widened by hand just to accept the new third argument.Verification
make check,make test(87),make check-fmt, repo-rootprettier --check,make bundle— all clean. NewexitError.test.tscovers the three message branches.pg_ctl's self-bounding was measured, not assumed: inpostgres:16-alpine, asmartstop held open by a live client andPGCTLTIMEOUT=5returned exit 1 at exactly 5 s. So dropping the SDK cap onpg_ctlcannot hang a backup — it converts aSIGKILLinto a realpg_ctlerror.SIGKILLlands on the host-sidestart-container subcontainer execwrapper.SIGKILLcannot be in that wrapper's forwarded-signal set (FWD_SIGNALS), and itsetnses into the subcontainer's PID namespace before spawning the real command, so thecpitself is not signalled — it is orphaned until the subcontainer is torn down. That is why the timeout note in the guide describes a timeout as "the SDK stopped waiting", not "the work stopped".Not verified end-to-end on a server with a multi-gigabyte dump; the failing path is a wall-clock timer, which is impractical to exercise here.