oxidecomputer / oxidecomputer/maghemite

BFD listener deletion/creation race

Open
#863 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bfd Bug mgd needs testing rust
Dominant language
Rust
Stars
94
Forks
6
Avg merge
1d 8h
Merged PRs (30d)
12

Description

The following is an issue identified by AI when I asked it to do an adversarial code review of #795. This was pre-existing in the sync/pthread impl, which is why it was not addressed as part of the async swap-out.

Problem

Removing the last peer for a listening address removes the listener from
dispatcher bookkeeping before the listener task has been aborted and
joined.

The DELETE handler releases the daemon mutex before awaiting the listener
shutdown handle. This is necessary to avoid awaiting while holding a
synchronous mutex, but it creates the following race:

  1. DELETE removes the final peer and receives a shutdown handle.
  2. Dispatcher bookkeeping no longer contains the listener.
  3. Before DELETE polls or completes listener shutdown, a concurrent PUT
    acquires the daemon lock.
  4. PUT attempts to bind the same address.
  5. The old listener task still owns the socket, so the bind fails with
    EADDRINUSE.
  6. PUT returns an internal error even though dispatcher state says the
    address is available.

Sequential clients that await DELETE before issuing PUT are protected,
but concurrent reconciliation is not.

Impact

Concurrent configuration reconciliation can lose the old session and
fail to install its replacement. The caller must retry an otherwise
valid configuration update.

Proposed direction

Represent listener shutdown in dispatcher state instead of removing the
resource before its lifetime ends.

Possible designs:

  • retain a ShuttingDown entry and have a new add await it;
  • serialize add/remove operations through a tokio::sync::Mutex held
    across listener shutdown;
  • move listener management into an actor that processes add/remove
    operations sequentially.

The synchronization should ideally cover complete session shutdown as
well, including RIB work, rather than only the UDP listener.

Testing

  1. Add a peer as the only user of a listener.
  2. Begin deletion while blocking listener shutdown.
  3. Concurrently attempt to add a peer using the same listen address.
  4. Verify the add waits rather than failing with AddrInUse.
  5. Release shutdown and verify the add succeeds.

Related issues

  • #787 originally tracked blocking synchronous teardown and noted that
    listener/state-machine workers were not reliably joined.
  • Related follow-up: prevent stale RIB writes from superseded sessions.

Context

A similar socket-lifetime race existed in the synchronous implementation.
#796 fixed the sequential DELETE-then-add case but not concurrent
reconciliation.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the DELETE and PUT handlers, dispatcher bookkeeping, and listener shutdown path described in the issue; review #795, #796, and #787 for existing lifecycle decisions. Reproduce the blocked-shutdown scenario with one peer, then verify that a concurrent add waits for shutdown and succeeds after release without an address-in-use error.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.