diff --git a/vendor/agent/flan_agent.c b/vendor/agent/flan_agent.c index 03518f1..ad749d2 100644 --- a/vendor/agent/flan_agent.c +++ b/vendor/agent/flan_agent.c @@ -106,7 +106,16 @@ extern const uint8_t *flan_restart_name(int32_t i, int64_t *len); /* What the listener thread hands the stopped game thread. One slot, because * only one thread is ever stopped. */ -static _Atomic int broken; /* the game thread is in the loop */ +/* A *depth*, not a flag. A thunk this loop runs may itself error, and the + * break loop that catches that one is nested inside this one — so a flag is + * wrong twice over: the inner loop clearing it on resume tells the world the + * program is running while the outer loop is still stopped, and every verb + * then answers "not stopped" while the outer loop spins forever with no + * protocol path out. Only kill recovered it. Counting fixes both. */ +static _Atomic int depth; +#define BREAK_MAX 8 /* deep enough to nest, shallow + * enough that a loop of breaks + * stops rather than grinds */ static char chosen[128]; static _Atomic int chosen_ready; static _Atomic int aborting; @@ -138,6 +147,11 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, } } fflush(stderr); + /* What the *outer* loop was reporting, restored on the way out: resuming an + * inner break must not leave the outer one describing a condition that has + * already been answered. */ + char outer_name[sizeof condition_name]; + memcpy(outer_name, condition_name, sizeof outer_name); { size_t k = namelen < 0 ? 0 : (size_t)namelen; if (k >= sizeof condition_name) k = sizeof condition_name - 1; @@ -146,7 +160,16 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, } /* Published last: the name has to be whole before anything advertises that * there is one to read. */ - atomic_store(&broken, 1); + if (atomic_fetch_add(&depth, 1) + 1 > BREAK_MAX) { + /* Printed without going near the hook: whatever is erroring is erroring + * inside the machinery that reports errors. */ + fflush(stdout); + fprintf(stderr, + "flan: %d nested break loops — giving up rather than spinning\n", + BREAK_MAX); + fflush(stderr); + exit(134); + } for (;;) { flan_agent_poll(); if (atomic_load(&aborting)) { @@ -155,16 +178,29 @@ static void break_loop(const uint8_t *name, int64_t namelen, void *condition, exit(134); } if (atomic_load(&chosen_ready)) { - int32_t ok = - flan_break_resume((const uint8_t *)chosen, (int64_t)strlen(chosen), xfer); + /* Claimed into a local and the flag cleared *before* the attempt. The + * other order loses a request that was already answered ok: a [restart] + * arriving during the attempt passes its own check, writes a new name + * and sets the flag, and the store below then erases it. Copying also + * keeps strlen off a buffer the listener may be writing. */ + char take[sizeof chosen]; + memcpy(take, chosen, sizeof take); atomic_store(&chosen_ready, 0); + int32_t ok = + flan_break_resume((const uint8_t *)take, (int64_t)strlen(take), xfer); if (ok) { - fprintf(stderr, "flan: resuming at restart %s\n", chosen); + fprintf(stderr, "flan: resuming at restart %s\n", take); fflush(stderr); - atomic_store(&broken, 0); + memcpy(condition_name, outer_name, sizeof condition_name); + /* Cleared with the resume: an abort that passed its check just as the + * game thread resumed would otherwise stay armed and kill the program + * at the *next* unhandled error, minutes later, in unrelated code, + * giving nobody the chance to choose. */ + atomic_store(&aborting, 0); + atomic_fetch_sub(&depth, 1); return; } - fprintf(stderr, "flan: no restart named %s is active\n", chosen); + fprintf(stderr, "flan: no restart named %s is active\n", take); fflush(stderr); } nanosleep(&step, NULL); @@ -226,6 +262,18 @@ static void reply(int fd, const char *s) { /* One connection, one line, one module. Loading here rather than in the game * thread is the whole reason this thread exists. */ static void serve(int fd) { + /* A deadline on the read. The accept loop is single-threaded and serves each + * connection inline, so a client that connects and then sends nothing - an + * editor killed mid-request, a daemon that crashed between connect and send - + * blocks every later request, including the [abort] that would end a stopped + * program. Worse, the requests the client had already given up on are served + * when its socket finally closes, so an abandoned abort can kill the program + * minutes later against a state that has moved on. Two seconds is generous + * for one line. */ + { + struct timeval tv = { 2, 0 }; + setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof tv); + } char line[4096]; size_t n = 0; for (;;) { @@ -251,7 +299,7 @@ static void serve(int fd) { * the one question an editor asks without knowing the state already, and * refusing it would leave nothing to poll. */ if (strcmp(line, "status") == 0) { - if (atomic_load(&broken)) { + if ((atomic_load(&depth) > 0)) { reply(fd, "stopped "); reply(fd, condition_name); reply(fd, "\n"); @@ -260,7 +308,7 @@ static void serve(int fd) { return; } if (strcmp(line, "restarts") == 0) { - if (!atomic_load(&broken)) { reply(fd, "err not stopped\n"); return; } + if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; } int32_t n = flan_restart_count(); for (int32_t i = 0; i < n; i++) { int64_t len = 0; @@ -274,7 +322,7 @@ static void serve(int fd) { return; } if (strncmp(line, "restart ", 8) == 0) { - if (!atomic_load(&broken)) { reply(fd, "err not stopped\n"); return; } + if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; } size_t k = strlen(line + 8); if (k == 0 || k >= sizeof chosen) { reply(fd, "err bad restart name\n"); return; } /* Checked here, against the stack the stopped thread is holding still, @@ -301,7 +349,7 @@ static void serve(int fd) { return; } if (strcmp(line, "abort") == 0) { - if (!atomic_load(&broken)) { reply(fd, "err not stopped\n"); return; } + if (!(atomic_load(&depth) > 0)) { reply(fd, "err not stopped\n"); return; } reply(fd, "ok\n"); atomic_store(&aborting, 1); return;