feat: cleanup repository before starting up IPFS #722

hacdias · 2018-11-28T15:31:20Z

If api file exists and it is the same as the api address defined in the config file, it is removed.

If api file exists and it is different from the one in the config file, the user will be asked if he wants to remove it or not.

Nothing os this will happen if there is a repo.lock.

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

lidel

@hacdias I think #724 may be a regression introduced by #711 and should be tackled as a part of this PR.

Basically, we need to detect and give user an option to cleanup after hard kill.
It is important because this will happen for regular users that don't play with api but it remains in place when someone kills daemon or PC has a force-shutdown eg. due to power outrage.

api and repo.lock are present but no daemon running AND api points at the same multiaddr as config
- CURRENT behaviour: fails with Error: connect ECONNREFUSED 127.0.0.1:5001
- TODO: Dialog to user about daemon being offline and give two options:
  - Run ipfs repo fsck and restart IPFS Desktop
  - Quit

Instead of doing manual cleanup, please take a look at re-using CLI command below.
Delegating cleanup to this command should be safer than removing locks by hand, as don't miss things like datastore/LOCK.

`ipfs repo fsck`

FYI there is a handy command for removing all locks:

'ipfs repo fsck' is a plumbing command that will remove repo and level db
lockfiles, as well as the api file. This command can only run when no ipfs
daemons are running.

It removes:

$IPFS_PATH/repo.lock
$IPFS_PATH/datastore/LOCK
$IPFS_PATH/api

olizilla · 2018-11-29T11:54:45Z

@lidel Do we have to prompt the user if we can tell it's from an unclean shutdown? I guess it's safer at this point to draw attention to it rather than hide it.

lidel · 2018-11-29T23:16:40Z

@olizilla I think prompt with two options (ipfs repo fsck or quit) is both safe and removes surprises.

It creates an opportunity to backup $IPFS_PATH before running ipfs repo fsck or to quit without doing anything and buys time to figure out what went wrong with <insert super custom setup user created to own demise> :)

hacdias · 2018-11-30T08:42:57Z

I can do the dialog as @lidel mentioned, it won't require many changes. It would be nice if js-ipfs-api supported that command. I'll need to run it directly.

olizilla · 2018-12-03T10:25:19Z

@hacdias we have the power to add commands to the http api! Raise an issue on https://github.com/ipfs/js-ipfs-http-client (new name for js-ipfs-api) and explain the use case, so we can get feedback and sanity check if there is any reason not to add it.

hacdias · 2018-12-03T10:59:12Z

@olizilla done: https://github.com/ipfs/js-ipfs-http-client/issues/901

lidel · 2018-12-03T11:03:24Z

Keep in mind we are talking about cleaning up lockfiles BEFORE go-ipfs starts, so http API can't be used anyway and running CLI command directly (with $IPFS_PATH set in env) is the only way to do it :)

hacdias · 2018-12-03T16:09:08Z

@olizilla @lidel so... the coded solution works, but I don't like it though. But it works. Could you try?

src/utils/daemon.js

lidel

I tested with api and repo.lock and they are deleted as expected:

$ inotifywait -m --format "%e %f" $IPFS_PATH
(...)
OPEN api
ACCESS api
CLOSE_NOWRITE,CLOSE api
MODIFY repo.lock
OPEN repo.lock
DELETE repo.lock
CLOSE_WRITE,CLOSE repo.lock

Digression: line break after try to looks odd:

ps. If I click "No", then I get the second error window:

I wonder if we should look into replacing "No" with "Quit" to avoid jumping between multiple error messages. More than one error dialog feels like a bad UX.

hacdias · 2018-12-03T21:00:11Z

@lidel I'm aware. I've a commit I didn't push yet, waiting for more feedback so we can close this soon.

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

lidel

Looks better!

Note . landed on the new line, probably because endline was read from api file:

Small fixes suggested below.

src/utils/errors.js

src/utils/daemon.js

src/utils/store.js

Co-Authored-By: hacdias <hacdias@gmail.com>

…nto api

olizilla · 2018-12-04T12:35:48Z

src/utils/daemon.js

+  const apiFile = join(path, 'api')
+
+  if (!await fs.pathExists(apiFile)) {
+    return true


Let's make the api for cleanup clearer. Having cleanup return true if it doens nothing is suprising. I think we need more info than just a boolean return value.

There are few other issues

const cfg = await fs.readJSON(cfgFile) could throw if the config isn't valid json. This function should be robust and try it's best to detect what's going on.

if (addr === cfgAddr && (lockFile || datastoreLock)) { I think the situatuon we're looking for is just "there is an api file and no lockFile and no datastroreLock.", then it's safe to just run fsck. If there is a lockfile and an apiFile, but we failed to connect to the api, it'd be ok to ask the user if we should try and fsck. We shouldn't offer to fsck the repo if there is alredy a daemon running that we just can't connect to for some reason.

Let's pull this function out to a helper detectOrphanApiFile that returns true if the api File is present and the same as the config default value, and that there is no repo.lock and no datastore/LOCK.

api and repo.lock are present but no daemon running AND api points at the same multiaddr as config

@lidel is it possible to assert that no daemon is running? is an ECONNREFUSED from the api enough?

I'd say ECONNREFUSED its the best platform-agnostic hint we can get.

Alternative is to execute platform-specific process enumeration(tasklist on windows, ps -A on linux, ps something on mac) to see if process is running, which may get blocked and not work reliably in more locked down systems.

olizilla · 2018-12-04T15:26:12Z

@Stebalien can we get a sanity check here please.

IPFS Desktop bundles go-ipfs and uses ipfsd-ctl. If the user also has go-ipfs installed manually and the deamon isn't allowed to shutdown cleanly, we've been seeing api files left in the repo, which leaves ipfsd-ctl trying to connect to a running ipfs daemon rather than spawning a new one. This PR is to implement a quick check for where we find an api file in the IPFS_HOME dir, to remove it by calling out to ipfs repo fsk. Is that a reasonable thing to do here?

I wasn't aware of the api file until we hit this issue. @lidel mentioned that he's found examples of people manually editing it as part or elborate schemes to access apis on remote machines.

Stebalien · 2018-12-05T00:24:19Z

So, I actually do this (and I know that others do as well). That is, I have an api file pointing to a daemon running as a different (restricted) user.

I'm not sure the best way to fix this. I guess one way is to check for a config file. That is:

If there's no config file, we have a "remote" daemon and should fail.
If there's a config file, we expect a local daemon.
Check if there's an open lock on repo.lock. If so, fail (there's a running daemon).
Otherwise, start the daemon anyways. DO NOT remove the api file, that's racy. Just start the daemon and let it take the repo lock. It should (I think) remove the api file itself.

Alternatively, instead of checking for config, we could check for version. Thoughts?

olizilla · 2018-12-06T09:47:55Z

Thanks @Stebalien. We're going to keep it simple for this pass. If ipfsd-ctl tries to connect to an existing api and fails, and there is an api file present, then we'll offer the user something like:

on the assumption that users who are setting api files directly will have a clue as to what is going on, while users that just have an unclean repo state have an option to fsck it, which is a command they may not have been aware of, and it may fix it for them.

olizilla · 2018-12-06T09:59:02Z

src/utils/daemon.js


-  if (!origins.includes('webui://-')) origins.push('webui://-')
-  if (!origins.includes('https://webui.ipfs.io')) origins.push('https://webui.ipfs.io')
+    if (!showRepoApiFileErrorMessage(ipfsd.repoPath, ipfsd.apiAddr)) {


This is neat, but it's now assuming that there is an api file rather than checking for it. Either we need to check for it so we can assert it in the error message, or we need to update the wording of the error message to something more like:

"IPFS Desktop failed to connect to an existing ipfs api at ${ipfsd.apiAddr}. This can happen if you run ipfs daemon manually and it is not shutdown cleanly. Would you like to try running ipfs repo fsck to remove the lock and api files from ${ipfsd.repoPath} and try again?

Just updated the error message.

src/utils/daemon.js

olizilla · 2018-12-06T10:02:28Z

src/utils/daemon.js

+
+export default async function (opts) {
+  const ipfsd = await spawn(opts)
+  await configure(ipfsd)


If ipfsd connects to an existing daemon process, and we change it's config, we would need to restart that process for the config to be applied.

@olizilla we're now editing the config file directly so we won't even be able to change the config of remote daemons. Perhaps it would be better to revert this change and use the API.

Ping @olizilla

@olizilla I'm merging this so we can speed up dev. This piece of code will be removed on #736 so let's not take too much time here.

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

feat: cleanup repository before starting up ipfs

63fb349

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

ghost assigned hacdias Nov 28, 2018

ghost added the in progress label Nov 28, 2018

hacdias changed the title ~~[WIP] feat: cleanup repository before starting up IPFS~~ feat: cleanup repository before starting up IPFS Nov 28, 2018

feat: ask to clean if they differ

4190b45

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

hacdias requested review from olizilla and lidel November 28, 2018 16:11

lidel mentioned this pull request Nov 28, 2018

Improve UX when $IPFS_PATH/api exists and API is offline #713

Closed

hacdias added this to the v1.0 milestone Nov 28, 2018

lidel mentioned this pull request Nov 28, 2018

npm start failed with Error: resource temporarily unavailable #724

Closed

lidel requested changes Nov 28, 2018

View reviewed changes

feat: run ipfs rpeo fsck

fc7ce9b

olizilla requested changes Dec 3, 2018

View reviewed changes

src/utils/daemon.js Outdated Show resolved Hide resolved

olizilla reviewed Dec 3, 2018

View reviewed changes

src/utils/daemon.js Show resolved Hide resolved

olizilla reviewed Dec 3, 2018

View reviewed changes

src/utils/daemon.js Show resolved Hide resolved

olizilla reviewed Dec 3, 2018

View reviewed changes

src/utils/daemon.js Outdated Show resolved Hide resolved

lidel reviewed Dec 3, 2018

View reviewed changes

hacdias added 2 commits December 4, 2018 10:12

feat: handle econnrefused

af78067

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

revert: package-lock

fb90571

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

lidel reviewed Dec 4, 2018

View reviewed changes

src/utils/errors.js Outdated Show resolved Hide resolved

lidel reviewed Dec 4, 2018

View reviewed changes

src/utils/daemon.js Outdated Show resolved Hide resolved

olizilla reviewed Dec 4, 2018

View reviewed changes

src/utils/daemon.js Show resolved Hide resolved

olizilla reviewed Dec 4, 2018

View reviewed changes

src/utils/store.js Outdated Show resolved Hide resolved

olizilla reviewed Dec 4, 2018

View reviewed changes

src/utils/store.js Outdated Show resolved Hide resolved

olizilla and others added 4 commits December 4, 2018 12:09

fix: reduce default keysize

8e1b964

Co-Authored-By: hacdias <hacdias@gmail.com>

feat: change ipfsCOnfig object name

4b0c426

Merge branch 'api' of https://github.com/ipfs-shipyard/ipfs-desktop i…

0580dcc

…nto api

fix: change store v

0dce216

olizilla reviewed Dec 4, 2018

View reviewed changes

fix: do not need to change origins on IPFS config

8884399

lidel mentioned this pull request Dec 5, 2018

Remove the need for setting CORS headers #728

Closed

hacdias added 4 commits December 5, 2018 13:43

revert prev commit

ce30819

dialog as soon as econnrefused

1ccc342

revert pkg lock

bc053f8

revert pkg lock

84188ff

olizilla reviewed Dec 6, 2018

View reviewed changes

src/utils/daemon.js Outdated Show resolved Hide resolved

olizilla reviewed Dec 6, 2018

View reviewed changes

Stebalien mentioned this pull request Dec 6, 2018

ipfs commands such as id fail when daemon has been started and stopped. ipfs/kubo#5784

Closed

hacdias added 2 commits December 7, 2018 15:42

fix: better error msg

97d13a6

License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>

fix: add quotes

e510c8a

hacdias merged commit 3b7c781 into master Dec 8, 2018

ghost removed the in progress label Dec 8, 2018

hacdias deleted the api branch December 8, 2018 14:00

hacdias mentioned this pull request Apr 17, 2020

feat: new ctl #1302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cleanup repository before starting up IPFS #722

feat: cleanup repository before starting up IPFS #722

hacdias commented Nov 28, 2018 •

edited

Loading

lidel left a comment

olizilla commented Nov 29, 2018

lidel commented Nov 29, 2018 •

edited

Loading

hacdias commented Nov 30, 2018

olizilla commented Dec 3, 2018

hacdias commented Dec 3, 2018

lidel commented Dec 3, 2018 •

edited

Loading

hacdias commented Dec 3, 2018

lidel left a comment •

edited

Loading

hacdias commented Dec 3, 2018

lidel left a comment •

edited

Loading

olizilla Dec 4, 2018

olizilla Dec 4, 2018

lidel Dec 4, 2018

olizilla commented Dec 4, 2018

Stebalien commented Dec 5, 2018

olizilla commented Dec 6, 2018

olizilla Dec 6, 2018

hacdias Dec 7, 2018

olizilla Dec 6, 2018

hacdias Dec 7, 2018

hacdias Dec 7, 2018

hacdias Dec 8, 2018

feat: cleanup repository before starting up IPFS #722

feat: cleanup repository before starting up IPFS #722

Conversation

hacdias commented Nov 28, 2018 • edited Loading

lidel left a comment

Choose a reason for hiding this comment

ipfs repo fsck

olizilla commented Nov 29, 2018

lidel commented Nov 29, 2018 • edited Loading

hacdias commented Nov 30, 2018

olizilla commented Dec 3, 2018

hacdias commented Dec 3, 2018

lidel commented Dec 3, 2018 • edited Loading

hacdias commented Dec 3, 2018

lidel left a comment • edited Loading

Choose a reason for hiding this comment

hacdias commented Dec 3, 2018

lidel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olizilla commented Dec 4, 2018

Stebalien commented Dec 5, 2018

olizilla commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hacdias commented Nov 28, 2018 •

edited

Loading

`ipfs repo fsck`

lidel commented Nov 29, 2018 •

edited

Loading

lidel commented Dec 3, 2018 •

edited

Loading

lidel left a comment •

edited

Loading

lidel left a comment •

edited

Loading