Skip to content

Commit 276af69

Browse files
ADFA-5333 fix(dashboard): harden re-own, auto-drain gate, and cancel
Review follow-ups on the background dashboard update: - Re-own can no longer start an unintended rebuild. DashboardRebuildService gains ACTION_ATTACH / attach(): the card re-owns an already-running rebuild by polling to completion, never POSTing /rebuild, so a rebuild that finishes in the gap between the status check and re-attach can't trigger a fresh run. - The reverse gate now covers auto-drains, not just user taps. ZimProvisioner and BooksProvisioner drain() and KolibriProvisioner canDrainNow() defer while DashboardRebuildService.isRunning(), so a banked wishlist can't auto-start a live download mid-rebuild (which the dash-node restart would disrupt). - Cancel is hardened on both ends. rebuild-dashboard.sh traps INT/TERM to on_cancel, which sets the status to idle before cleanup, so a canceled build never leaves a wedged "running" (promote stays signal-proof). The cancel endpoint verifies the recorded pid is actually rebuild-dashboard.sh (via
1 parent dc6a74e commit 276af69

7 files changed

Lines changed: 65 additions & 25 deletions

File tree

‎controller/app/src/main/java/org/iiab/controller/kolibri/presentation/KolibriProvisioner.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,9 @@ public static boolean hasPending(Context ctx) {
5959
*/
6060
static boolean canDrainNow(Context ctx) {
6161
if (org.iiab.controller.install.presentation.ModuleQueueRepository.get().isRunning()
62-
|| MapsProvisioner.hasPending(ctx)) {
63-
Log.d(TAG, "kolibri drain blocked: proot (runrole) work is pending/running");
62+
|| MapsProvisioner.hasPending(ctx)
63+
|| org.iiab.controller.redesign.DashboardRebuildService.isRunning()) { // ADFA-5333
64+
Log.d(TAG, "kolibri drain blocked: proot (runrole) or dashboard-update work is in flight");
6465
return false;
6566
}
6667
// ADFA-5074: unfinished work only. This read a merely registered session, so a

‎controller/app/src/main/java/org/iiab/controller/redesign/BooksProvisioner.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,8 +41,9 @@ public static void drain(Context ctx) {
4141
// background processes and concurrent REST work is a recipe for corruption. Defer REST while a
4242
// module-queue (proot) job is pending or running; a later drain pass picks it up once idle.
4343
if (org.iiab.controller.install.presentation.ModuleQueueRepository.get().isRunning()
44-
|| MapsProvisioner.hasPending(ctx)) {
45-
Log.d(TAG, "books drain deferred: proot (runrole) work is pending/running");
44+
|| MapsProvisioner.hasPending(ctx)
45+
|| DashboardRebuildService.isRunning()) { // ADFA-5333: a live dashboard update restarts dash-node
46+
Log.d(TAG, "books drain deferred: proot (runrole) or dashboard-update work is in flight");
4647
return;
4748
}
4849
// ADFA-4954 (ADR-4954 D8): the live REST streams also serialize against each other.

‎controller/app/src/main/java/org/iiab/controller/redesign/DashboardDetailFragment.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ private void resolveInitialUpdatingState() {
147147
if (!isAdded()) return;
148148
if (DashboardRebuildService.STATE_RUNNING.equals(state) && !DashboardRebuildService.isRunning()) {
149149
setUpdating(true);
150-
DashboardRebuildService.start(ctx);
150+
DashboardRebuildService.attach(ctx); // re-own without risking a fresh rebuild
151151
}
152152
}
153153
@Override public void onErr(String message) { /* box stopped/offline — nothing in flight to show */ }

‎controller/app/src/main/java/org/iiab/controller/redesign/DashboardRebuildService.java‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ public final class DashboardRebuildService extends Service {
5151
private static final int NOTIFICATION_ID = 10; // app-global; distinct from the other services
5252

5353
public static final String ACTION_START = "org.iiab.controller.DASHBOARD_UPDATE_START";
54+
/** ADFA-5333: re-own an already-running rebuild (poll only, never POST a new one). */
55+
public static final String ACTION_ATTACH = "org.iiab.controller.DASHBOARD_UPDATE_ATTACH";
5456
/** ADFA-5333: request a clean cancel (from the notification action or the card button). */
5557
public static final String ACTION_CANCEL = "org.iiab.controller.DASHBOARD_UPDATE_CANCEL";
5658
/** App-internal broadcast on every state change so a visible card can reflect it live. */
@@ -72,14 +74,20 @@ public final class DashboardRebuildService extends Service {
7274
private boolean started = false; // one rebuild per service instance; ignore re-delivered starts
7375
private boolean cancelling = false; // a cancel request is in flight; ignore repeats
7476

75-
/** Kick the background live update. Safe to call again while running — the box reports 409 and the
76-
* service just keeps polling the in-flight rebuild (so a card can re-own a lost session by calling
77-
* this after it sees the box still rebuilding). */
77+
/** Kick a NEW background live update (POST + poll). */
7878
public static void start(Context ctx) {
7979
ContextCompat.startForegroundService(ctx,
8080
new Intent(ctx, DashboardRebuildService.class).setAction(ACTION_START));
8181
}
8282

83+
/** Re-own a rebuild that is ALREADY running on the box (e.g. our process was killed mid-update): poll
84+
* it to completion and restore the notification, but never POST a new rebuild — so a card that sees
85+
* the box still building can attach without risking a second run if the original just finished. */
86+
public static void attach(Context ctx) {
87+
ContextCompat.startForegroundService(ctx,
88+
new Intent(ctx, DashboardRebuildService.class).setAction(ACTION_ATTACH));
89+
}
90+
8391
@Override public void onCreate() { super.onCreate(); createNotificationChannel(); }
8492
@Nullable @Override public IBinder onBind(Intent intent) { return null; }
8593

@@ -90,15 +98,22 @@ public int onStartCommand(Intent intent, int flags, int startId) {
9098
// — otherwise the system raises "did not call startForeground". It's idempotent, so satisfy the
9199
// contract first.
92100
startForeground(NOTIFICATION_ID, buildOngoing());
93-
if (ACTION_CANCEL.equals(intent != null ? intent.getAction() : null)) { requestCancel(); return START_NOT_STICKY; }
101+
final String action = intent != null ? intent.getAction() : null;
102+
if (ACTION_CANCEL.equals(action)) { requestCancel(); return START_NOT_STICKY; }
94103
if (started) return START_NOT_STICKY;
95104
started = true;
96105
RUNNING = true;
97106
broadcastState(STATE_RUNNING);
98-
DashboardClient.rebuildStart(new DashboardClient.RebuildStartCb() {
99-
@Override public void onStarted(boolean alreadyRunning) { pollStatus(); }
100-
@Override public void onErr(String message) { finish(STATE_ERROR, R.string.k2go_dash_live_start_failed); }
101-
});
107+
if (ACTION_ATTACH.equals(action)) {
108+
// Re-own: the box is already building, so just poll to completion (a first poll that finds
109+
// done/error finishes at once — no new rebuild is ever launched).
110+
pollStatus();
111+
} else {
112+
DashboardClient.rebuildStart(new DashboardClient.RebuildStartCb() {
113+
@Override public void onStarted(boolean alreadyRunning) { pollStatus(); }
114+
@Override public void onErr(String message) { finish(STATE_ERROR, R.string.k2go_dash_live_start_failed); }
115+
});
116+
}
102117
return START_NOT_STICKY;
103118
}
104119

‎controller/app/src/main/java/org/iiab/controller/redesign/ZimProvisioner.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,9 @@ public static void drain(Context ctx) {
4242
// background processes; concurrent REST work risks corruption). Defer ZIM while a module-queue
4343
// (proot) job is pending or running; a later drain pass runs it once proot is idle.
4444
if (org.iiab.controller.install.presentation.ModuleQueueRepository.get().isRunning()
45-
|| MapsProvisioner.hasPending(ctx)) {
46-
Log.d(TAG, "zim drain deferred: proot (runrole) work is pending/running");
45+
|| MapsProvisioner.hasPending(ctx)
46+
|| DashboardRebuildService.isRunning()) { // ADFA-5333: a live dashboard update restarts dash-node
47+
Log.d(TAG, "zim drain deferred: proot (runrole) or dashboard-update work is in flight");
4748
return;
4849
}
4950
// ADFA-4954 (ADR-4954 D8): the live REST streams also serialize against each other.

‎static/dashboard/routes.ts‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -316,16 +316,32 @@ apiRouter.post('/system/dashboard/rebuild/cancel', (_req: Request, res: Response
316316
// can't signal it. Do NOT claim success or touch the status — the app can retry in a moment.
317317
res.status(409).json({ error: 'cancel not ready', cancelled: false }); return;
318318
}
319+
// Verify the pid is actually OUR rebuild script before signaling — never SIGTERM a recycled/unrelated
320+
// pid. /proc/<pid>/cmdline is NUL-separated; a substring match on the script name is enough.
321+
let cmdline = '';
322+
try { cmdline = fs.readFileSync(`/proc/${pid}/cmdline`, 'utf8'); } catch { /* process gone / no proc */ }
323+
const healStale = (): void => {
324+
try { fs.writeFileSync(REBUILD_STATUS_FILE, 'idle'); } catch { /* best effort */ }
325+
try { fs.rmdirSync(REBUILD_LOCK_DIR); } catch { /* maybe already gone */ }
326+
try { fs.unlinkSync(REBUILD_PHASE_FILE); } catch { /* maybe already gone */ }
327+
try { fs.unlinkSync(REBUILD_PID_FILE); } catch { /* maybe already gone */ }
328+
};
329+
if (cmdline === '' && !fs.existsSync(`/proc/${pid}`)) {
330+
// The run already exited but left stale state (status 'running' with a dead pid) — heal it and
331+
// report success; there is nothing left to stop.
332+
healStale();
333+
res.status(200).json({ ok: true, cancelled: true }); return;
334+
}
335+
if (!cmdline.includes('rebuild-dashboard.sh')) {
336+
// The pid is alive but was recycled by an unrelated process — refuse to signal it.
337+
res.status(409).json({ error: 'stale pid', cancelled: false }); return;
338+
}
319339
try {
320-
// Signal the whole detached session group; its EXIT/TERM trap purges staging + removes lock/phase/pid.
340+
// Signal the whole detached session group; the script's on_cancel/EXIT trap sets status idle and
341+
// purges staging + lock/phase/pid. We also write idle + best-effort rmdir here as belt-and-suspenders.
321342
try { process.kill(-pid, 'SIGTERM'); } catch { /* group already gone */ }
322343
try { process.kill(pid, 'SIGTERM'); } catch { /* leader already gone */ }
323-
// We own the status file, so reset it to idle (a stopped build is "nothing running", not an error).
324-
// Best-effort lock rmdir in case the trap is slow, so a fresh rebuild can start without wedging.
325-
try { fs.writeFileSync(REBUILD_STATUS_FILE, 'idle'); } catch { /* best effort */ }
326-
try { fs.rmdirSync(REBUILD_LOCK_DIR); } catch { /* trap likely removed it */ }
327-
try { fs.unlinkSync(REBUILD_PHASE_FILE); } catch { /* trap may have removed it */ }
328-
try { fs.unlinkSync(REBUILD_PID_FILE); } catch { /* trap may have removed it */ }
344+
healStale();
329345
res.status(200).json({ ok: true, cancelled: true });
330346
} catch (e: any) {
331347
res.status(500).json({ error: e?.message || 'cancel failed', cancelled: false });

‎tools/rebuild-dashboard.sh‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,17 @@ verify_live() {
7272
}
7373
cleanup() { [ -n "${TESTPID:-}" ] && kill "$TESTPID" 2>/dev/null || true; purge_staging; rmdir "$LOCK" 2>/dev/null || true; rm -f "$PHASE" "$PIDFILE" 2>/dev/null || true; }
7474
fail() { log "FAIL: $*"; set_status "error"; cleanup; exit 1; }
75+
# ADFA-5333: a cancel (SIGTERM/INT during the building phase) self-heals the state — set the status to
76+
# idle BEFORE cleanup so a killed build can never leave a wedged "running" (the endpoint also writes idle,
77+
# but this makes it true regardless of whether the endpoint completes). Ignored during promote (trapped off).
78+
on_cancel() { log "canceled"; set_status "idle"; cleanup; exit 0; }
7579

7680
# Single-flight: mkdir is atomic.
7781
mkdir "$LOCK" 2>/dev/null || { echo "another rebuild is running" >&2; exit 3; }
7882
# ADFA-5333: clean staging + phase/pid on ANY exit, including a cancel signal (TERM/INT). Killing this
7983
# session during steps 1-3 therefore leaves nothing behind and the live dashboard untouched.
80-
trap cleanup EXIT INT TERM
84+
trap cleanup EXIT
85+
trap on_cancel INT TERM
8186
: > "$LOG" 2>/dev/null || true
8287
set_status "running"
8388
set_phase "building"
@@ -174,6 +179,7 @@ else
174179
rm -rf "$BACKUP"
175180
set_status "error"
176181
fi
177-
# Promote window done — nothing left touches the live dashboard, so signals may be handled normally again.
178-
trap cleanup EXIT INT TERM
182+
# Promote done: the terminal status (done/error) is already written and only cleanup remains, so keep
183+
# INT/TERM ignored — a late signal must NOT run on_cancel and overwrite that status with "idle". The EXIT
184+
# trap still runs cleanup on the way out.
179185
cleanup

0 commit comments

Comments
 (0)