Skip to content

fix(server): recover from unclean SIP2 shutdowns - #129

Open
rerowep wants to merge 1 commit into
inveniosoftware-contrib:masterfrom
rerowep:fix/selfcheck-shutdown
Open

fix(server): recover from unclean SIP2 shutdowns#129
rerowep wants to merge 1 commit into
inveniosoftware-contrib:masterfrom
rerowep:fix/selfcheck-shutdown

Conversation

@rerowep

@rerowep rerowep commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

A SIP2 server killed before it can deregister itself leaves its record marked running with a stale pid. Server.create refuses to start on that flag alone, so the next start raises ServerAlreadyRunning and, under restart: on-failure, the container retries forever until the record is cleared by hand with invenio selfcheck stop -d.

The datastore had no socket timeouts, so a write to an unreachable Redis blocked forever. Deregistering is part of shutting down, so that wait outlived the container grace period and made a SIGKILL, and therefore a stale record, the normal outcome of stopping Redis and the SIP2 server together. Connections now default to a 5s timeout, overridable through query arguments on SIP2_DATASTORE_REDIS_URL.

The stop signal handler performed that deregistration itself: network I/O from a signal handler, which blocks unrecoverably and stacks another blocked call on the next signal. It also closed the selector without stopping the loop, so run() crashed into the closed selector and its finally clause repeated the whole teardown, doubling the work on a deadline that was already being missed. The handler now only raises a flag; the loop polls it and closes once, in normal context, and closing can no longer raise.

Nothing reclaimed a stale record. Registrations carry the hostname beside the pid, and a running record is reclaimed when its owner is provably gone: another machine or container, no such process, or a pid recycled after a reboot whose process postdates the registration it would own. Records predating hostnames fall back to the local pid check, so the upgrade does not strand the record it finds. Starting with --force takes over from a server that is still alive.

Finally, stop marked the record down only when the process had already vanished, leaving the success path to the handler that hangs; it now does so unconditionally. It also signalled the recorded pid without checking that it belongs to this host, and on a record carrying no pid psutil.Process(None) resolved to the command itself. Both are refused now, AccessDenied is reported, and stop waits for the process to go.

A SIP2 server killed before it can deregister itself leaves its record
marked `running` with a stale pid. Server.create refuses to start on
that flag alone, so the next start raises ServerAlreadyRunning and,
under `restart: on-failure`, the container retries forever until the
record is cleared by hand with `invenio selfcheck stop -d`.

The datastore had no socket timeouts, so a write to an unreachable
Redis blocked forever. Deregistering is part of shutting down, so that
wait outlived the container grace period and made a SIGKILL, and
therefore a stale record, the normal outcome of stopping Redis and the
SIP2 server together. Connections now default to a 5s timeout,
overridable through query arguments on SIP2_DATASTORE_REDIS_URL.

The stop signal handler performed that deregistration itself: network
I/O from a signal handler, which blocks unrecoverably and stacks
another blocked call on the next signal. It also closed the selector
without stopping the loop, so run() crashed into the closed selector
and its finally clause repeated the whole teardown, doubling the work
on a deadline that was already being missed. The handler now only
raises a flag; the loop polls it and closes once, in normal context,
and closing can no longer raise.

Nothing reclaimed a stale record. Registrations carry the hostname
beside the pid, and a `running` record is reclaimed when its owner is
provably gone: another machine or container, no such process, or a pid
recycled after a reboot whose process postdates the registration it
would own. Records predating hostnames fall back to the local pid
check, so the upgrade does not strand the record it finds. Starting
with --force takes over from a server that is still alive.

Finally, stop marked the record down only when the process had already
vanished, leaving the success path to the handler that hangs; it now
does so unconditionally. It also signalled the recorded pid without
checking that it belongs to this host, and on a record carrying no pid
psutil.Process(None) resolved to the command itself. Both are refused
now, AccessDenied is reported, and stop waits for the process to go.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rerowep rerowep self-assigned this Jul 28, 2026
@rerowep
rerowep requested review from PascalRepond, jma and lauren-d July 28, 2026 06:18

@jma jma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've accepted, but I don't know enough well this module to be relevant.

@PascalRepond PascalRepond left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same as @jma. Wait for @lauren-d review to be sure.

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.

3 participants